[IPython-dev] Code review (mostly for Fernando)
ellisonbg at gmail.com
Sat Sep 18 12:10:46 EDT 2010
I just looked through everything and it looks great. Thanks for doing
this. Things are looking really good.
On Sat, Sep 18, 2010 at 3:13 AM, Fernando Perez <fperez.net at gmail.com> wrote:
> 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
> I documented the execution semantics a lot more:
> 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.
>>> 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 =
>> Ah, OK, cool.
>>> 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.]
>>> 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.
>> 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.
>>> Add blank line right before def _ofind(...)
>> yup :)
>>> 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:
> Done as discussed: user_variables, user_expressions. Documented and
> changed the method api to match the messaging spec.
> Thanks again!
Brian E. Granger, Ph.D.
Assistant Professor of Physics
Cal Poly State University, San Luis Obispo
bgranger at calpoly.edu
ellisonbg at gmail.com
More information about the IPython-dev