Code review comment for lp:~ipython-dev/ipython/set_trace

Revision history for this message
Ondrej Certik (ondrej-certik) wrote :

> On Mon, Jul 27, 2009 at 9:08 PM, Ondrej Certik<email address hidden> wrote:
> >> If it simplifies embedding, then yes, I think we would consider it.  But,
> it
> >> should go into Shell.py, which I have just refactored for the 0.11 release
> >> (won't go into 0.10).  Are you OK waiting for the 0.11 for this - it will
> be
> >> much cleaner to implement then.
> >
> > Sure, I can move it whenever is the best and sure I can wait for 0.11. Are
> you ok with me adding a stub into the __init__.py, so that this can be invoked
> just by:
> >
> > import IPython; IPython.embed()
>
> Yes, I'm also OK with having a simplified embed() be part of the
> top-level public API. But it should at least take a few arguments.
> Basically you are 'folding' into a single embed() call the creation of
> an IPShellEmbed instance and its activation:
>
> embed() <==> IPShellEmbed()()
>
> But if we look at the signatures for IPShellEmbed's constructor and
> activation, we see:
>
> def __init__(self,argv=None,banner='',exit_msg=None,rc_override=None,
> user_ns=None):
> def __call__(self,header='',local_ns=None,global_ns=None,dummy=None):
>
> so I'd suggest making the embed() signature something that reasonably
> merges the two:
>
> def embed(argv=None,banner='',exit_msg=None,rc_override=None,
> header='',local_ns=None,global_ns=None)
>
> where the relevant arguments are fed through to either the __init__ or
> the __call__ as appropriate.
>
> This makes it consistent with the API it's wrapping for convenience
> (only where it makes sense, so things like dummy which are meant for
> the stateful part of the object aren't needed in this form).
>
> With such caveats, I have no objections to exposing this at the
> top-level. It is a very useful feature, so having the one-liner
> being even easier to remember is a net win as far as I see it.

Excellent, I am glad we are on the same boat now. I will play with this and try to produce a new patch. I will create the method embed(<args as you suggested>), add it to Shell.py and import it from __init__.py.

Ondrej

« Back to merge proposal