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

Revision history for this message
Brian Granger (ellisonbg) wrote :

A few comments:

* for 0.11, the actual embed implementation should go into ipapi or shell,
which ever makes most sense. shell is now very cleaned up and it really
only has things that qualify as being "public". Thus, it might make sense
for it to go there.
* There should definitely be an import of embed into IPython.__init__.
Things like this shield users (at some level) from changes to the
implementation of things.
* The first time embed gets called it should create the IPShellEmbedded
instance and *save* it as some global like, shell._embedded_shell (this
makes me think ipapi makes more sense!) and then call it. Future calls to
embed should just call the same instance over again. I want to make sure
that embed can be called multiple times.
* Trunk will be ready for this as of Friday this week.

Cheers,

Brian

On Tue, Jul 28, 2009 at 2:51 AM, Ville M. Vainio <email address hidden> wrote:

> On Tue, Jul 28, 2009 at 4:50 AM, Ondrej Certik<email address hidden> wrote:
>
> >> you access to stuff in IPython package (import ipy; ipy.embed() )
> >
> > Do you mean to provide the root level module in IPython, or in all my
> other programs?
>
> I meant creating a module that you install in your site packages,
> which only does:
>
> from IPython.ipapi import embed
>
> This seems more hygienic than importing the name to IPython package.
> Ultimately, I don't think the root level package should import
> anything else - but we can easily agree to disagree on this, I don't
> really care about it one way or another - just think it's a tad messy.
>
> --
> Ville M. Vainio
> http://tinyurl.com/vainio
> https://code.launchpad.net/~ipython-dev/ipython/set_trace/+merge/8855<https://code.launchpad.net/%7Eipython-dev/ipython/set_trace/+merge/8855>
> You are subscribed to branch lp:ipython.
>

--
Brian E. Granger, Ph.D.
Assistant Professor of Physics
Cal Poly State University, San Luis Obispo
<email address hidden>
<email address hidden>

« Back to merge proposal