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

If interactive, use IPython if available #189

Closed
wants to merge 1 commit into from

Conversation

bwrsandman
Copy link

No description provided.

@reinout
Copy link
Contributor

reinout commented Jul 1, 2015

This pull request has been around for almost a year. Some brainstorm-like questions to get it going again:

  • This pretty much hard-codes ipython into buildout. Do we really want that?
  • Does that mean other projects want us to hard-code them, too?
  • What about Support of PYTHONSTARTUP environment variable #235? Could that python startup environment variable be used instead?

@reinout
Copy link
Contributor

reinout commented Jul 1, 2015

In any case, you'll need to add a test and some documentation (in some cases, just a changelog entry might be enough).

@bwrsandman
Copy link
Author

  • This pretty much hard-codes ipython into buildout. Do we really want that?

Not exactly hard coding a dependency, it just uses it if it's available. The way I wrote it might look like it needs IPython, but it's just checking if it's there and if not, run the same code as before.

  • Does that mean other projects want us to hard-code them, too?

It may not be the best option. It may be better to give this str parameters, so other projects can use it appropriately.

It could. However, #235 wouldn't solve the problem completely. If I understand it correctly, it will still run

__import__("code").interact(banner="", local=globals())

Whereas IPython needs to run:

IPython.start_ipython(user_ns=globals())

An env variable could be made such as PYTHONCONSOLE or something like it.

What do you think?

@reinout
Copy link
Contributor

reinout commented Jul 3, 2015

Ok, I like the idea. ipython is used a lot, so a special case for that seems reasonable.

The try/except/else makes me feel a little bit stupid though. I never understand such a statement :-) Would something like this be clearer? It'd make my head hurt less, but it might be a bit over the top.

try:
    import IPython
except ImportError:
    IPython = None
if IPython:
    the_ipython_thingy()
else:
    the_regular_python_thingy()

A better alternative might be to just add a simple comment line right after the "except" saying "no ipython detected, running the regular python interpreter". Yes, I like that one more :-)

@bwrsandman bwrsandman force-pushed the ipython branch 3 times, most recently from ce75a99 to 6c5d9d8 Compare July 3, 2015 20:21
@bwrsandman
Copy link
Author

How about this? I reduced the number of lines by using __import__ the same way the regular python interpreter does.

@reinout
Copy link
Contributor

reinout commented Jul 3, 2015

Looks much nicer, imho, thanks.

I don't think there's much that can be tested about this. I assume you've tried it out already with ipython in place and verified it worked?

Do you have a suggestion for a changelog entry as documentation? (You can also merge master and add a changelog entry yourself, but I suspect just typing something here is easier :-) )

@bwrsandman
Copy link
Author

I wouldn't mind adding a unittest to avoid possible future regressions. I just need to familiarize myself with the test suite in this project.

Changelog entry:

Interactive shell now drops into IPython interpreter if available.

@reinout
Copy link
Contributor

reinout commented Jul 3, 2015

If you're adding a unittest, please merge master first.

Test suite: mostly doctests. So... they do the job and the test coverage is pretty good. But it can be hard to get your head around them. The thing you're testing? Doesn't look too bad. There are 2 or 3 tests that directly test the creation of such an interpreter script. Perhaps only test that there's a string "IPython" in them?

To get the tests running, just follow the short instructions in DEVELOPERS.txt. A quick makefile command will set you up for testing within a few minutes.

@bwrsandman
Copy link
Author

Thank you. It will probably wait until next week.

@reinout
Copy link
Contributor

reinout commented Jul 5, 2015

@jimfulton: sounds fair.

So perhaps simply an ipython-interpreter next to interpreter? This might actually be better as the existing patch only imports ipython, but doesn't itself ensure that it is available, So also some left-over ipython from a two-year-old experiment might be imported and run.

=> Add a separate option and it'll be easier to use and it'll be more explicit.

@veloutin
Copy link

veloutin commented Jul 6, 2015

I would name it interactive-interpreter, default to python/None and allow specifying ipython to get the new behavior.

@bwrsandman
Copy link
Author

I'm sorry where is this interpreter option you're talking about? Do you mean inside of the .cfg itself? Wouldn't that force all environments to use ipython?

What if I like developing and prototyping with ipython for the tab completion but I don't want to deploy ipython in production.

@reinout
Copy link
Contributor

reinout commented Jul 6, 2015

@jimfulton is right: using ipython if some old version of ipython is on the sys.path somewhere is a bad idea.

I don't like having an extra option, however. There are enough options as-is. And having buildout itself depend on something? I doubt that's something we want to do.

To me, there seems an easy solution: don't try/except to import ipython, but look if it is in the working set. The "try/except import" might give you a weird old version that you installed years ago. Checking the working set tells you if the user explicitly wanted ipython to be available. Which probably means, in 95% of the cases, that you preferably want to use ipython as interactive shell.

The advantage of the "working set" approach is that you don't have an extra option and that you also don't have an extra buildout dependency.

@jimfulton: how does that sound?

@bwrsandman: don't worry, I'm willing to help with getting the working set stuff in :-) (Warning: I'm on holiday in 1.5 weeks, though).

@jimfulton
Copy link
Member

On Mon, Jul 6, 2015 at 5:52 PM, Reinout van Rees notifications@github.com
wrote:

@jimfulton https://github.com/jimfulton is right: using ipython if some
old version of ipython is on the sys.path somewhere is a bad idea.

I don't like having an extra option, however. There are enough options
as-is. And having buildout itself depend on something?

How would buildout depend on ipython (other than having code to import it)?

I doubt that's something we want to do.

To me, there seems an easy solution: don't try/except to import ipython,
but look if it is in the working set. The "try/except import" might give
you a weird old version that you installed years ago. Checking the working
set tells you if the user explicitly wanted ipython to be available.
Which probably means, in 95% of the cases, that you preferably want to use
ipython as interactive shell.

The advantage of the "working set" approach is that you don't have an
extra option and that you also don't have an extra buildout dependency.

@jimfulton https://github.com/jimfulton: how does that sound?

So if something I depend on happens to depend on ipython, my environment
changes?
I don't know how likely that is.

I wonder if this can be better done with a recipe, possibly with some help
from the misnamed easy_install module.

Jim

Jim Fulton
http://www.linkedin.com/in/jimfulton

@reinout
Copy link
Contributor

reinout commented Jul 6, 2015

This got me thinking. To me, it seems like it is already possible without any modification! I just tried it locally and it works like a charm.

[buildout]
eggs = yourproject_which_includes_dependencies
parts = console_scripts

[console_scripts]
recipe = zc.recipe.egg
dependent-scripts = true
#^^^ This generates bin/something scripts for all entry points in all dependencies.
eggs =
    ${buildout:eggs}
    ipython
    zest.releaser
    pyflakes

This way you'll get a bin/pyflakes and a bin/ipython and whatever you want with the correct sys.path set. That dependent-scripts=true setting really helps a lot.

@bwrsandman: can you try if that solves your use case?

@bwrsandman
Copy link
Author

I tried on https://github.com/anybox/anybox.recipe.odoo which is what I use every day, it doesn't seem to do anything. But it might just be a particularity of that implementation.

@leorochael
Copy link
Contributor

Hi @bwrsandman, anybox.recipe.odoo indeed does somethings differently, but to get an ipython_odoo with the same python packages available as the python_odoo that it generates, all you need to do is add ipython to that parts eggs setting and add ipython also to the openerp_scripts section.

Please find a minimalistic example here:
https://gist.github.com/leorochael/888f1a728b227676244c

Since it's so easy to get an IPython interpreter in the expected buildout context, I think we should close this PR.

@bwrsandman
Copy link
Author

I tried it out. I see two issues.

Major: the ipython script doesn't export the globals user_ns=globals()
Minor: It would be nice to edit the banner

@leorochael
Copy link
Contributor

@bwrsandman, You can do both with the arguments option of the openerp_scripts setting.

I've updated the gist adding also bpython and passing the session object.

Modifying the banner is left as a (much more challenging) exercise for the reader.

@bwrsandman
Copy link
Author

@leorochael thank you for the tip, this PR is no longer necessary.

@bwrsandman bwrsandman closed this Jul 27, 2015
@bwrsandman bwrsandman deleted the ipython branch July 27, 2015 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants