Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Namespaces #1939

Merged
merged 3 commits into from Jun 26, 2012
Merged

Namespaces #1939

merged 3 commits into from Jun 26, 2012

Conversation

minrk
Copy link
Member

@minrk minrk commented Jun 13, 2012

This is the first step towards #1537

Further (more important) things I haven't worked out yet:

  1. single import for all display-related functions (currently in core.display, lib.display)
  2. InteractiveShellEmbed, InteractiveShell, etc. somewhere so that simple entry points / handles are readily accessible.

Possible options:

  1. top-level IPython.display has all our display apis
  2. IPython.api defines our general public interface - display, anything else
  3. IPython.api is a package, where we put various categorized public APIs (e.g. IPython.api.display)

Any suggestions? This seems a pretty general issue we have, where we haven't really thought about public APIs at all, aside from magics and methods on the InteractiveShell object. Users need to be much too familiar with our code layout to use it.

@@ -3,7 +3,7 @@
__docformat__ = "restructuredtext en"

#-------------------------------------------------------------------------------
# Copyright (C) 2008-2011 The IPython Development Team
# Copyright (C) 2012 The IPython Development Team
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2012 -> 2008 ? If I remember correctly we said original year no ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, of course. Sorry I got it backwards. Thanks!

@fperez
Copy link
Member

fperez commented Jun 13, 2012

Glad to see a start on this. My vote would be to keep this for 0.14, as even though our api right now has precisely the problems you point out here, I think we can live with the awkwardness for a bit longer so we really nail this. In my mind, 0.14 will be the rampup to 1.0 status, and the perfect time to plan this.

I'd like to ask @rkern for his thoughts on how this has worked out for Enthought. I know you guys have spent a fair bit of time on this question, and gone back and forth a bit. Thoughts?

@fperez
Copy link
Member

fperez commented Jun 13, 2012

BTW, from the user's perspective, I think I'd favor just some top-level packages, like IPython.display. But that makes the directory layout actually somewhat more confusing, unless we stuff them into __init__. Having a .api helps with that, but I find the approach kind of ugly...

@minrk
Copy link
Member Author

minrk commented Jun 13, 2012

Pretty much exactly my thoughts so far.

@fperez
Copy link
Member

fperez commented Jun 13, 2012

OK, targeted for 0.14 for now, while we give others a chance to pitch in with ideas. This one will take some calm thought...

@rkern
Copy link
Contributor

rkern commented Jun 13, 2012

We never bothered much with categorized APIs, as we were mostly just trying to migrate away from having everything exposed in the __init__.py rather than carefully designing things from the ground up. The places where we do that (eg. chaco.tools.api separate from chaco.api) were already subpackages in their own right. We also have a lot of other modules in our packages, so categorized API modules would be difficult to distinguish from implementation modules. You don't have this problem at the top level, which is good.

I think the categorized API modules directly under IPython would work well. Just don't introduce implementation modules at the top level. Then people reading the code can just make a rule in their head that all of the .py files at the top level expose public APIs to be used, and the subpackages are just where the implementation is. People who are using IPython imports but not reading the code will just be following the documentation which just won't mention the subpackages in normal examples, so there isn't much room for confusion there, either.

Oh, and IPython developers should make a rule in their head to never use those API modules inside the IPython codebase.

@ellisonbg
Copy link
Member

I am +1 on all of @rkern suggestions, especially his last comment about IPython devs not ever using the API modules inside the IPython codebase.

@ellisonbg
Copy link
Member

Although upon thinking more, I think our subpackages do provide a nice place for public API level things. In fact we are already using IPython.parallel in this manner:

from IPython.parallel import Client

Why not:

from IPython.core import HTML

If we start to put top-level API modules in place, what do we do about subpackages that already have that name and are functioning in that role?

@takluyver
Copy link
Member

core doesn't seem a very descriptive location for the HTML display class. It also means that that code would be loaded even in the plain terminal app, which has no use for it. Perhaps an IPython.display namespace?

@ellisonbg
Copy link
Member

Yes, in some ways IPython.display makes sense, but my point is that if we go this way our "API" will be in both 1) top-level API-only modules such as IPython.display and 2) in the __init__ files of various subpackages such as IPython.parallel. I just find that a bit confusing.

@minrk
Copy link
Member Author

minrk commented Jun 17, 2012

I agree, most of these are easily resolved by working with the __init__.py in the various packages, as is done in parallel, and added to config/zmq here.

There are two issues I think we have to resolve which require a little bit more thought:

  1. We have lib.display and core.display. Nobody should have to know which one has which display functions. We could just do away with the lib.display file, or have an API package in some manner that consolidates these (and possibly others).
  2. Using the embedded terminal requires importing IPython.frontend.terminal.embed.InteractiveShellEmbed. There should be no public APIs more than three levels deep, and no common ones more than two.

@ellisonbg
Copy link
Member

On Sat, Jun 16, 2012 at 5:02 PM, Min RK
reply@reply.github.com
wrote:

I agree, most of these are easily resolved by working with the __init__.py in the various packages, as is done in parallel, and added to config/zmq here.

There are two issues I think we have to resolve which require a little bit more thought:

  1. We have lib.display and core.display. Nobody should have to know which one has which display functions. We could just do away with the lib.display file, or have an API package in some manner that consolidates these (and possibly others).

Good point. So maybe in this situation we do want an
IPython/display.py API file.

  1. Using the embedded terminal requires importing IPython.frontend.terminal.embed.InteractiveShellEmbed. There should be no public APIs more than three levels deep, and no common ones more than two.

Would IPython.frontend.embed not work?


Reply to this email directly or view it on GitHub:
#1939 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Member Author

minrk commented Jun 26, 2012

continuing conversation from #1537 here. It seems like we want this in 0.13, so if there's anything more that we definitely want to do right now before merging, or anything here that should be removed, etc.

@fperez
Copy link
Member

fperez commented Jun 26, 2012

I've also drifted to +1 for this going in, just to record things here. It may be partial, but esp. the fixes to the display public names I think are very important, as that's something very widely used by the public.

@fperez
Copy link
Member

fperez commented Jun 26, 2012

I'm tentatively moving it to 0.13 so the filtered issues list shows us a complete picture of 0.13.

@fperez
Copy link
Member

fperez commented Jun 26, 2012

If nobody pipes in with negative feedback, let's merge this then. Will give it the rest of the day. I'm thinking on trying to cut the release before the weekend.

@takluyver
Copy link
Member

It looks OK to me. The main thing I'd check with this kind of restructuring is that we're not forcing the import of modules that we wouldn't normally load, which could slow down startup. It doesn't look like the changes here do that.

@fperez
Copy link
Member

fperez commented Jun 26, 2012

Indeed, that's a good thing to keep in mind, @takluyver. Thanks for the review, merging then.

fperez added a commit that referenced this pull request Jun 26, 2012
Start cleaning up our namespaces to expose a more user-friendly layout of our class hierarchy.

This includes now a top-level `IPython.display` module with all display classes and functions, as well as easier-to-find objects in `IPython.config`.
@fperez fperez merged commit 970025e into ipython:master Jun 26, 2012
@minrk minrk deleted the namespaces branch March 31, 2014 23:36
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Start cleaning up our namespaces to expose a more user-friendly layout of our class hierarchy.

This includes now a top-level `IPython.display` module with all display classes and functions, as well as easier-to-find objects in `IPython.config`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants