8000 Improve our error messages for non compatibility. by Carreau · Pull Request #9900 · ipython/ipython · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Improve our error messages for non compatibility. #9900

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

Merged
merged 1 commit into from
Aug 23, 2016

Conversation

Carreau
Copy link
Member
@Carreau Carreau commented Aug 23, 2016

I would like to have the transition (even on master) as smooth as possible, with good error message.

There is a large number of instruction that explain how to install ipython from source on the internet, and we can make an effort for all the python 2 users around that will encounter that to have a seamless transition or at least a good error message.

Otherwise we will get a lot of bug report from users.

One of the thing we can do, is to actually open an issue just for that, and point to this issue number in the error messag, plus update this issue with instructions.

The following is not super friendly:

$ python2 -m pip install .
Processing /Users/bussonniermatthias/dev/ipython
    Complete output from command python setup.py egg_info:
    ERROR: IPython requires Python version 3.3 or above.

    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in /var/folders/ld/jy0c_5d91rl_s3jqh74tmqn00000gn/T/pip-_zeInP-build/

@Carreau
Copy link
Member Author
Carreau commented Aug 23, 2016

@willingc I would appreciate any of your insight on the error message.

I'd like @fperez and @ellisonbg input on this process.

I even think that a real multine error that also give some (basic) reasoning as why we stop support, and how to get the 5.x branch woudl be welcome.

@fperez
Copy link
Member
fperez commented Aug 23, 2016

I think this PR is already a big improvement. I wonder if it would be useful to host a permanent page with a bit more detail, and include a bit.ly URL to it in the message. Something like bit.ly/ipy23 or similar that we can control and redirect in the future if our website layout ever changes.

But this is already a good start!

@Carreau
Copy link
Member Author
Carreau commented Aug 23, 2016

I think we can do better, I don't know if we have a bit.ly account, and that would be fine with me to have an updatable page. .

$ python2 -m IPython
Traceback (most recent call last):
  File "/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 163, in _run_module_as_main
    mod_name, _Error)
  File "/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 111, in _get_module_details
    __import__(mod_name)  # Do not catch exceptions initializing package
  File "IPython/__init__.py", line 61, in <module>
    """)
ImportError:

============== IPython 6.0+ requires Python version 3.3 or above. =============
|
| For a version compatible with Python 2.7, please install the 5.x Long Term
| Support version.
|
| If you are encountering this error message you are likely trying to be using
| IPython from source. You need to checkout the remote 5.x branch, likely:
|
|   $ git fetch origin
|   $ git checkout -b origin/5.x
|
| If you encounter this error message with a regular install of IPython, then you
| likely need to update your package manager, for example if you are using `pip`
| check the version of pip with
|
|   $ pip --version
|
| You will need to update pip to the version 8.2 or greater. If you are not using
| pip, please inquiry with the maintainers of the package for your package
| manager.
|
| For more information see one of our blog posts:
|
|     http://blog.jupyter.org/2016/07/08/ipython-5-0-released/
|

@Carreau
Copy link
Member Author
Carreau commented Aug 23, 2016

Also once we agree on the message, we need to duplicate it in setup.py.

@fperez
Copy link
M 8000 ember
fperez commented Aug 23, 2016

I'm just not convinced that an exception traceback is the place for long-form instructions. That's why I think that it's better to point to the source of the problem and link to a persistent resource with more details.

Putting tutorials into tracebacks is, I think, stretching too far the intent of an error message.

if sys.version_info < (3,3):
raise ImportError('IPython 6.0+ requires Python version 3.3 or above. '
'For a version compatible with Python 2.7, please install'
' the 5.x Long Term Support version.')
Copy link
Member

Choose a reason for hiding this comment

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

raise ImportError('IPython 6.0+ does not support Python 2.7, 3.0, 3.1, or 3.2. '
                             'When using Python 2.7, please install IPython 5.x LTS Long Term Support version. '
                             'Beginning with IPython 6.0, Python 3.3. and above is required.') 

Since the error message will be going to someone that does not have 3.3+ installed, the above error message using the following: 1. Tell them the error. 2. Tell them the resolution (use 5.x LTS). 3. Tell them why.

@Carreau
Copy link
Member Author
Carreau commented Aug 23, 2016

That's one of my other questions, should we only raise (just for imports), or should we print + sys.exit(nonzero) ? Note that we are doing that in setup.py, so the error message can easily be as long as we want, it is not a traceback.

@willingc
Copy link
Member

Perhaps for simplicity, it would be good to have a brief error message as I suggest in the code review. One sentence that could be added to the error message that refers to a changelog. For example,

raise ImportError('IPython 6.0+ does not support Python 2.7, 3.0, 3.1, or 3.2. '
                             'When using Python 2.7, please install IPython 5.x LTS Long Term Support version. '
                             'Beginning with IPython 6.0, Python 3.3. and above is required.'
                             'See CHANGELOG.md and IPython documentation for details.') 

We could add a CHANGELOG.md file at the repo's root with @Carreau's longer error text re: IPython 6.x and link to the docs from there. The CHANGELOG.md would be easy to update as needed.

@fperez
Copy link
Member
fperez commented Aug 23, 2016

A Changelog file is really tricky to maintain in a distributed VC environment as it easily leads to merge conflicts. And I'd rather not start a ChangeLog again (we used to have one), since now our release process instead generates its equivalent out of the github PR and issue list.

But, iterating on this idea: how about instead we add a short section on this topic at the end of the main README.md? This is likely important enough to warrant mention there for a while to come, and that would mean it's available to the users always right next to them (if they are installing from source, by definition they have the README.md).

So, we could keep the short & sweet error message as per @willingc's suggestion in the code, and a bit more info at the end of the README.

That would, I think, satisfy the constraints, obviate the need for managing a separate URL, and pick up on the Changelog idea without the issues that specific kind of file brings up.

Thoughts?

@Carreau
Copy link
Member Author
Carreau commented Aug 23, 2016

Done.

@willingc
Copy link
Member

Iteration FTW. @fperez Great suggestion.

@Carreau
Copy link
Member Author
Carreau commented Aug 23, 2016

Iteration FTW. @fperez Great suggestion.

Travis seem to be on the path of happiness.

Hope you had a nice flight back @willingc BTW. Looking forward to have you here !

@willingc willingc merged commit 59b3863 into ipython:master Aug 23, 2016
@Carreau Carreau deleted the update-no-2x branch August 23, 2016 19:35
@Carreau
Copy link
Member Author
Carreau commented Aug 23, 2016

Thanks !

@fperez
Copy link
Member
fperez commented Aug 23, 2016

Great, thx!

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.

3 participants
0