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

don't use get_ipython from builtins in library code #3286

Merged
merged 6 commits into from
May 20, 2013

Conversation

minrk
Copy link
Member

@minrk minrk commented May 7, 2013

mainly adds IPython.get_ipython() as a library-level handle that does the same thing.

We still use get_ipython from builtins in two ways:

  • We use it a lot in tests and I haven't changed those, but I can if we want to.
    The same instance mechanism should still work there, it doesn't need to be a special case.
  • Some code uses get_ipython from globals as a test for 'are we in IPython?`,
    which seems appropriate, though we can perhaps define a single,
    official test for that and make sure we use that everywhere.

This also fixes the issue discussed in #3285

minrk added 2 commits May 6, 2013 17:41
library entry point for get_ipython

does not rely on injection into globals
@takluyver
Copy link
Member

We used to have a variable called something like __IPYTHON__ just to indicate that code was running in IPython. I forget what happened to it.

@minrk
Copy link
Member Author

minrk commented May 7, 2013

If I remember correctly, we removed it in 0.11 but put it back in 0.12.

@ellisonbg
Copy link
Member

I am +1 on these changes, but I would rather create a new module than reusing ipapi which has been marked for death for a really long time. How about core.getipython?

@minrk
Copy link
Member Author

minrk commented May 10, 2013

What is the reasoning for removing a module that does one thing (just because it has a 'deprecation' note in the docstring, though we have actually been using it all along) and creating a new module that does the exact same thing?

@ellisonbg
Copy link
Member

Mostly history. ipapi was originally the mutant nasty that almost killed IPython. I spent most of a summer trying to exorcise it. Originally, ipapi did much more that get_ipython does - that is what is being deprecated. For me there are two reasons to change the name:

  • I don't think the name ipapi communicates what get_ipython actually does.
  • I want to completely close the book on the ipapi experience.
  • The official way of getting this should be at IPython.get_ipython. The function is simple enough that I am not sure it even warrants being anywhere other than IPython.__init__.py.

"""

#-----------------------------------------------------------------------------
# Copyright (C) 2008-2011 The IPython Development Team
# Copyright (C) 2013 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.

Why? I thought it was standard practice to leave the dates for which copyright applies (basically from file creation onwards)...

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what we have been doing all over IPython - I believe you were a part of that conversation many months ago. Updating the copyright to the current date when we update a file.

Copy link
Member

Choose a reason for hiding this comment

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

No, I guess it was keeping the starting date, omitting the -END one.
2008-2011 -> 2008[Nothing] so you don't have to update the second year every year.
But It is always confusing me too.

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha, thanks. in any case, this is a new file now, so 2013 seems right.

I still think per-file copyright is utterly pointless (like putting copyright notice on every page of a book).

@fperez
Copy link
Member

fperez commented May 10, 2013

I also agree that ipapi is a poor and nebulous name: our public api is large and complex, so there's no point in thinking that we can shove all of it into some mega-import wrapper called ipapi or somesuch.

I'm slightly -1 on putting actual code in __init__ files in general, out of a sense of cleanliness and keeping the code closer to its tests, etc.

As for the more generic question of how to detect that IPython is active, I think it's an important discussion we should try to sort out for 1.0. I don't think this PR is the place for it: perhaps it's better to wrap up this cleanup and think about that separately, as that question has lots of little edge cases we should consider carefully.

@minrk
Copy link
Member Author

minrk commented May 11, 2013

move core.ipapi to core.getipython and call this done, then? Since we are moving, I would remove the deprecation warning on get, since it would be in a new location.

@fperez
Copy link
Member

fperez commented May 11, 2013

Yup, looks good to me. Now's the time to break backwards compatibility :)

One last thing before merging: a note in the docs, in the backw. compat. section, indicating this break (the flat out removal of ipapi). Then we're good.

@minrk
Copy link
Member Author

minrk commented May 11, 2013

the core.ipapi deprecation has always seemed weird to me - we moved IPython.ipapi to IPython.core.ipapi, breaking all old code, at the same time as deprecating it, so there should never have been any code that ever used core.ipapi, since it was never anything but deprecated.

@ellisonbg
Copy link
Member

I think this looks good now, is there anything left to finish?

minrk added a commit that referenced this pull request May 20, 2013
don't use `get_ipython` from builtins in library code

mainly adds IPython.get_ipython() as a library-level handle that does the same thing.
@minrk minrk merged commit b82bef4 into ipython:master May 20, 2013
@minrk minrk deleted the get_ipython branch May 20, 2013 16:36
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
don't use `get_ipython` from builtins in library code

mainly adds IPython.get_ipython() as a library-level handle that does the same thing.
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

5 participants