[IPython-dev] Code review (mostly for Fernando)

[Fernando Perez]

Hey,

I've just finished up the main cleanups post-review, thanks again!

In the process I ended up implementing call signature in the calltips,
along with at least a few unittests for the object inspector (going
with the idea of trying to add unittests to any new code as much as
possible).  Now that we have calltips, I was really missing the
signature in them :)

Evan had mentioned adding more sophisticated calltips that would carry
syntax highlighting of the current argument being typed; all the info
is in there and we can extend to that later. For now I'm just
formatting the signature as a plain string, and if someone can
contribute the fancier version later, it's just a matter of changing
one flag to prevent plain string conversion and doing something
better.  But even the plain string version is very useful, I think.

On Tue, Sep 14, 2010 at 10:59 PM, Fernando Perez
<Fernando.Perez at berkeley.edu> wrote:

>> Does the kernel know anything about cell mode?  If so, we need to go over this
>> more carefully. Added later: I now see that the kernel does know about cell
>> mode. We should talk more about this.
>
> Yes, what we need to do is rationalize the number of 'run*' entry
> points we have.  Those have accreted over time, starting with the old,
> convoluted and unnecessary approach that we inherited from code.py
> (push()->runsource()->runcode()).

[...]

I documented the execution semantics a lot more:

http://ipython.scipy.org/doc/nightly/html/development/messaging.html#execute

We still have more work to do on this front, as discussed.  I'll try
to crack the off-by-one prompt bug and organize the core execution
loop better this weekend.

> Yes, paste is not ideal and I don't like it either.  I was thinking
> 'pastefig' earlier today: it's pretty explicit and matches the
> sound/feel of 'savefig', already in mpl.  How does that sound?

Done, it's now pastefig()

>> Let's rename "instant_death" to "now" everywhere as people are used to this
>> word in this context (sudo shutdown -h now)
>
> Sounds good, I'll do that in a quick commit now (no pun intended :).
> I wasn't very happy with that name either, but couldn't think of a
> better one.

Done.

>> The initialization of the input_splitter in init_instance_attrs should be
>> moved to somewhere else. Basically, init_instance_attrs should eventually be
>> retired and anything that is traited should not be in there.  You can have
>> traits autocreate the input splitter by doing::
>>
>> input_splitter =
>> Instance('IPython.core.inputsplitter.IPythonInputSplitter',(),{})
>
> Ah, OK, cool.

Done.

>> http://github.com/ipython/ipython/commit/e7e389c1052ceda15656f84edf8cff41f3640992
>>
>> I know this is not nice to say, but I think we should use a different data
>> structure than a NamedTuple or dict here. Basically, we want the message
>> protocol to use universal data structures. The closest thing to a NamedTuple
>> is a list of tuples:
>>
>> data = [('type_name',type_name),('base_class',base_class), etc.]
>>
>> This will be both ordered and easy for both Python and Javascript to
>> parse and print.  It will also be easy for Python frontends to put the data
>> back into a NamedTuple if needed.  But the wire protocol should be universal.
>> If we just send the dict, Javascript clients will loose the ordering info.
>
> Totally agreed, no worries.  Right now only a python client could
> reconstruct the order by importing the fields list, by switching to a
> list-of-pairs we'll make it language-agnostic.

I removed the namedtuple but left it as a dict for now.  The
underlying data structure was a dict and that goes fine over json, so
there's no problem with that.  Even for non-python clients, it may be
more convenient to just leave the json dict alone and ship (once) the
list of ordered names. Since that may turn out to be the cleaner
solution in the long run, and leaving the dicts alone actually made
the *current* code we do have a fair bit simpler, I decided against
the list-of-tuples for now.  Go with the simplest solution that works
and only add complexity later if we do really have a use for it.

>> http://github.com/ipython/ipython/commit/4e3685be1b53680b1894da6bba96141ed582b123
>>
>> Add blank line right before def _ofind(...)
>
> yup :)

Done.

>> http://github.com/ipython/ipython/commit/2be9133b2a3035f51932b3ebd9ac054a9b3e28ba
>>
>> Maybe rename get_user_variables to something that reflects we are getting
>> the repr's of the variables?
>
> We have these two that return reprs:
>
> get_user_variables
> eval_expressions

Done as discussed: user_variables, user_expressions.  Documented and
changed the method api to match the messaging spec.

Thanks again!

Cheers,

f