Merge lp:~ipython-dev/ipython/set_trace into lp:ipython/0.11
- set_trace
- Merge into trunk
Status: | Rejected | ||||
---|---|---|---|---|---|
Rejected by: | Brian Granger | ||||
Proposed branch: | lp:~ipython-dev/ipython/set_trace | ||||
Merge into: | lp:ipython/0.11 | ||||
Diff against target: | None lines | ||||
To merge this branch: | bzr merge lp:~ipython-dev/ipython/set_trace | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Fernando Perez | Needs Information | ||
Jörgen Stenarson | Needs Fixing | ||
Ville M. Vainio | Needs Fixing | ||
Review via email: mp+8855@code.launchpad.net |
Commit message
Description of the change
Ondrej Certik (ondrej-certik) wrote : | # |
Ville M. Vainio (villemvainio) wrote : | # |
This should probably not be in __init__.py. It is intended for those who embed ipython, i.e. public api. Therefore it should be in ipapi.py.
Ondrej Certik (ondrej-certik) wrote : | # |
> This should probably not be in __init__.py. It is intended for those who embed
> ipython, i.e. public api. Therefore it should be in ipapi.py.
Many thanks for the review, I'll fix it. Should I fix it by adding a new commit, or by changing the last commit (how can this be done with bzr?)?
Ville M. Vainio (villemvainio) wrote : | # |
> > This should probably not be in __init__.py. It is intended for those who
> embed
> > ipython, i.e. public api. Therefore it should be in ipapi.py.
>
> Many thanks for the review, I'll fix it. Should I fix it by adding a new
> commit, or by changing the last commit (how can this be done with bzr?)?
Just continue committing to this branch and push. You can't amend the old commit.
Ondrej Certik (ondrej-certik) wrote : | # |
> > > This should probably not be in __init__.py. It is intended for those who
> > embed
> > > ipython, i.e. public api. Therefore it should be in ipapi.py.
> >
> > Many thanks for the review, I'll fix it. Should I fix it by adding a new
> > commit, or by changing the last commit (how can this be done with bzr?)?
>
> Just continue committing to this branch and push. You can't amend the old
> commit.
Ok, I have moved it to IPython/ipapi.py. So now it has to be imported with:
IPython.
do you think I can even import it in IPython/
One more question --- I would like to change my email in the last two commits, is there a way to do it in bzr? It uses ondrej@crow, but I'd like to use my real email address.
- 1175. By Ondrej Certik
-
Implements IPython.set_trace() for starting ipython inplace
Important is that you can access all your local and global symbols, e.g. it's
just like if you didimport pdb
pdb.set_trace()Thus the name.
- 1176. By Ondrej Certik
-
set_trace() moved from __init__.py to ipapi.py
Ondrej Certik (ondrej-certik) wrote : | # |
> > > > This should probably not be in __init__.py. It is intended for those who
> > > embed
> > > > ipython, i.e. public api. Therefore it should be in ipapi.py.
> > >
> > > Many thanks for the review, I'll fix it. Should I fix it by adding a new
> > > commit, or by changing the last commit (how can this be done with bzr?)?
> >
> > Just continue committing to this branch and push. You can't amend the old
> > commit.
>
> Ok, I have moved it to IPython/ipapi.py. So now it has to be imported with:
>
> IPython.
>
> do you think I can even import it in IPython/
> that?
>
> One more question --- I would like to change my email in the last two commits,
> is there a way to do it in bzr? It uses ondrej@crow, but I'd like to use my
> real email address.
I have fix the emails, see the ipython list for how I did it.
- 1177. By Ondrej Certik
-
Imports the Shell variable, which is no longer imported by default
Jörgen Stenarson (jorgen-stenarson) wrote : | # |
Ondrej,
I have seen two problems with the docstring, it is not valid rst and will not render ok with sphinx.
The first code example looks like it should be an example at a common python prompt, but then it needs a proper >>> prompt.
The second code example will render on a single line. To get an in-line code example use a double colon at the end of the previous line.
/Jörgen
Fernando Perez (fdo.perez) wrote : | # |
Hi Ondrej,
sorry for not having worked on this earlier...
What I don't understand is what the point of this feature is, when we already have the embedding classes as explained here:
http://
Your code basically reduces the code needed to embed ipython from 3 to 2 lines, but the existing machinery actually does it with much more flexibility. So I'd like to understand the justification for this feature better before considering it for merging. Ville's and Jorgen's comments still apply, but I'm actually wondering if we need this *at all*. It seems to me that we already have tools for this, though I may well be missing something and would be happy to reconsider.
Ondrej Certik (ondrej-certik) wrote : | # |
> Hi Ondrej,
>
> sorry for not having worked on this earlier...
>
> What I don't understand is what the point of this feature is, when we already
> have the embedding classes as explained here:
>
> http://
>
> Your code basically reduces the code needed to embed ipython from 3 to 2
> lines, but the existing machinery actually does it with much more flexibility.
> So I'd like to understand the justification for this feature better before
> considering it for merging. Ville's and Jorgen's comments still apply, but
> I'm actually wondering if we need this *at all*. It seems to me that we
Thanks for the suggestion, this oneliner does exactly what I want:
from IPython.Shell import IPShellEmbed; IPShellEmbed()()
The only reason why I devised my line and then contributed a patch is that noone has either replied or suggested anything better so far, up until you.
> already have tools for this, though I may well be missing something and would
> be happy to reconsider.
There is only one thing that I am still missing --- It's really annoying to always type:
from IPython.Shell import IPShellEmbed; IPShellEmbed()()
and it still annoying to type:
import IPython; IPython.
I would much prefer to just type:
import IPython; IPython.embed()
I use this feature *very* often. Would you consider such a patch?
Ondrej
Brian Granger (ellisonbg) wrote : | # |
> Thanks for the suggestion, this oneliner does exactly what I want:
>
> from IPython.Shell import IPShellEmbed; IPShellEmbed()()
>
Thanks for catching this Fernando. Until recently, I haven't been very
familiar with our embedding code.
> The only reason why I devised my line and then contributed a patch is that
> noone has either replied or suggested anything better so far, up until you.
>
> > already have tools for this, though I may well be missing something and
> would
> > be happy to reconsider.
>
> There is only one thing that I am still missing --- It's really annoying to
> always type:
>
> from IPython.Shell import IPShellEmbed; IPShellEmbed()()
>
> and it still annoying to type:
>
> import IPython; IPython.
>
> I would much prefer to just type:
>
> import IPython; IPython.embed()
>
What would that do? I
> I use this feature *very* often. Would you consider such a patch?
>
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.
Cheers,
Brian
> Ondrej
> --
> https:/
> 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>
Ondrej Certik (ondrej-certik) wrote : | # |
> > Thanks for the suggestion, this oneliner does exactly what I want:
> >
> > from IPython.Shell import IPShellEmbed; IPShellEmbed()()
> >
>
> Thanks for catching this Fernando. Until recently, I haven't been very
> familiar with our embedding code.
>
>
> > The only reason why I devised my line and then contributed a patch is that
> > noone has either replied or suggested anything better so far, up until you.
> >
> > > already have tools for this, though I may well be missing something and
> > would
> > > be happy to reconsider.
> >
> > There is only one thing that I am still missing --- It's really annoying to
> > always type:
> >
> > from IPython.Shell import IPShellEmbed; IPShellEmbed()()
> >
> > and it still annoying to type:
> >
> > import IPython; IPython.
> >
> > I would much prefer to just type:
> >
> > import IPython; IPython.embed()
> >
>
> What would that do? I
From the user perspective it would be exactly equivalent to:
from IPython.Shell import IPShellEmbed; IPShellEmbed()()
> > I use this feature *very* often. Would you consider such a patch?
> >
>
> 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()
?
Ondrej
Fernando Perez (fdo.perez) 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_
def __call_
so I'd suggest making the embed() signature something that reasonably
merges the two:
def embed(argv=
header=
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.
Thanks!
f
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_
> user_ns=None):
> def __call_
>
> so I'd suggest making the embed() signature something that reasonably
> merges the two:
>
> def embed(argv=
> header=
>
> 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
Ville M. Vainio (villemvainio) wrote : | # |
On Tue, Jul 28, 2009 at 3:12 AM, Fernando Perez<email address hidden> wrote:
>> 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.
IPython.ipapi is the top level public api, not IPython. Starting to
shove stuff to IPython package namespace is messy, and the line should
be drawn somewhere.
If the aim is to reduce typing, provide a root level module that gives
you access to stuff in IPython package (import ipy; ipy.embed() )
--
Ville M. Vainio
http://
Fernando Perez (fdo.perez) wrote : | # |
On Tue, Jul 28, 2009 at 1:39 AM, Ville M. Vainio<email address hidden> wrote:
> IPython.ipapi is the top level public api, not IPython. Starting to
> shove stuff to IPython package namespace is messy, and the line should
> be drawn somewhere.
This is meant for 0.11, where we are refactoring the entire layout of
the package. I have no problem whatsoever with some commonly used
names being exposed at the top level, and embedding is certainly a
common enough task.
I actually probably look up the 'standard one-liner' from the docs
more than anything else, so I already see value in having it as an
instantly rememberable/
> If the aim is to reduce typing, provide a root level module that gives
> you access to stuff in IPython package (import ipy; ipy.embed() )
Nope. This, we will most certainly not do, at all.
Packages should not go about polluting the top level *Python* pacakge
namespace, which is a shared resource, for little things like this.
Ondrej Certik (ondrej-certik) wrote : | # |
> On Tue, Jul 28, 2009 at 3:12 AM, Fernando Perez<email address hidden> wrote:
>
> >> 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.
>
> IPython.ipapi is the top level public api, not IPython. Starting to
> shove stuff to IPython package namespace is messy, and the line should
> be drawn somewhere.
>
> If the aim is to reduce typing, provide a root level module that gives
Yes, the aim is to reduce typing (e.g. I was using IPython.
> 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?
The way I see it is to put one line into IPython/__init__.py
from Shell import embed
(or something similar). I don't think it's messy --- it's something that users will call frequently.
Ondrej
Ville M. Vainio (villemvainio) 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://
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._
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://
> https:/
> 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>
Ondrej Certik (ondrej-certik) 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._
> 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.
Cool, please ping me when your branch is ready, so that I can take it and update the embed() code according to this thread.
Ondrej
Brian Granger (ellisonbg) wrote : | # |
Ondrej,
Trunk is completely ready for you to finish your work in this branch. At this point we should be able to merge this quickly.
Brian Granger (ellisonbg) wrote : | # |
These capabilities are now in trunk. See the function IPython.embed.
> Ondrej,
>
> Trunk is completely ready for you to finish your work in this branch. At this
> point we should be able to merge this quickly.
Unmerged revisions
- 1177. By Ondrej Certik
-
Imports the Shell variable, which is no longer imported by default
- 1176. By Ondrej Certik
-
set_trace() moved from __init__.py to ipapi.py
- 1175. By Ondrej Certik
-
Implements IPython.set_trace() for starting ipython inplace
Important is that you can access all your local and global symbols, e.g. it's
just like if you didimport pdb
pdb.set_trace()Thus the name.
Preview Diff
1 | === modified file 'IPython/__init__.py' |
2 | --- IPython/__init__.py 2009-03-17 03:05:22 +0000 |
3 | +++ IPython/__init__.py 2009-07-15 23:32:08 +0000 |
4 | @@ -59,6 +59,31 @@ |
5 | |
6 | import Shell |
7 | |
8 | +def set_trace(): |
9 | + """ |
10 | + Starts the IPython shell inplace. |
11 | + |
12 | + Example: |
13 | + |
14 | + >> import IPython |
15 | + >> IPython.set_trace() |
16 | + |
17 | + The user namespace is set to the union of globals() and locals(), e.g. |
18 | + calling set_trace() is exactly equivalent to: |
19 | + |
20 | + import IPython |
21 | + IPython.Shell.IPShell(user_ns=dict(globals(), **locals())).mainloop() |
22 | + |
23 | + but the actual implementation of set_trace() needs to use the inspect |
24 | + module, because it needs to access the parents frame. |
25 | + |
26 | + """ |
27 | + import inspect |
28 | + frame = inspect.currentframe().f_back |
29 | + globals = frame.f_globals |
30 | + locals = frame.f_locals |
31 | + Shell.IPShell(user_ns=dict(globals, **locals)).mainloop() |
32 | + |
33 | # Release data |
34 | from IPython import Release # do it explicitly so pydoc can see it - pydoc bug |
35 | __author__ = '%s <%s>\n%s <%s>\n%s <%s>' % \ |
Hi,
here is my set_trace() patch. Please review it and let me know what you think. See also these emails to the ipython list:
http:// mail.scipy. org/pipermail/ ipython- dev/2009- June/005191. html
see also the whole thread:
http:// mail.scipy. org/pipermail/ ipython- dev/2009- June/thread. html#5191