gh-72327: Add help message for pip in REPL by tomviner · Pull Request #8536 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@tomviner tomviner commented

Jul 28, 2018

•

edited by bedevere-appbot

Choose a reason for hiding this comment

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

Nice! This feels similar in spirit to the change we made for print statement syntax, but easier to handle at the excepthook level since we don't need to worry about the non-interactive case.

Lib/site.py Outdated Show resolved Hide resolved

if typ is SyntaxError and (
"pip install" in value.text or "pip3 install" in value.text
):
value.msg = ("The Python package manager (pip) can only be used"

Choose a reason for hiding this comment

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

Since there is a chance of false detection, it is worth to include the original message in the result.

Choose a reason for hiding this comment

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

The original message is just "invalid syntax", so to put that back in would require going from what we have now:

  File "<string>", line 1
    pip install qwerty
              ^
SyntaxError: The Python package manager (pip) can only be used from outside of Python.
Please try the `pip` command in a separate terminal or command prompt.

to something like the more expansive:

  File "<string>", line 1
    pip install qwerty
              ^
SyntaxError: invalid syntax

Note: The Python package manager (pip) can only be used from outside of Python.
Please try the `pip` command in a separate terminal or command prompt.

I'm happy with either.

Choose a reason for hiding this comment

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

It is not always "invalid syntax".

>>>     x = 'pip install'
  File "<stdin>", line 1
    x = 'pip install'
    ^
IndentationError: unexpected indent

Choose a reason for hiding this comment

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

I think this is why the original suggestion from ncoghlan has is SyntaxError rather than isinstance, so subclasses (IndentationError & TabError) don't get caught.

Choose a reason for hiding this comment

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

It is not always "invalid syntax" even if the type is SyntaxError.

>>> x = ['pip install')
  File "<stdin>", line 1
    x = ['pip install')
                      ^
SyntaxError: closing parenthesis ')' does not match opening parenthesis '['

Also I think that we perhaps can use __notes__ here.

All commit authors signed the Contributor License Agreement.

@tomviner, please sign the CLA again for both of your email addresses by clicking the button above. It is now required sign it for register email addresses instead of b.p.o accounts.

@iritkatriel, could you please take a look? Is this a correct use of __notes__?

@ncoghlan, as the main proponent of such changes, could you please look at it again? Does the message look good to you?

This PR is stale because it has been open for 30 days with no activity.

My apologies, @tomviner, but it seems that there are now conflicts with other changes made in the last 9 months. Could you please resolve them?

@ncoghlan, could you please check that this PR completely implements your request?

I did not see anything wrong, so if this is what we need, I'm going to merge this PR after resolving conflicts.

Contributor

@ncoghlan ncoghlan left a comment • Loading

Choose a reason for hiding this comment

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

Thanks for the ping @serhiy-storchaka, this had definitely dropped off my review radar.

I think we still want one slight API tweak, so enablerlcompleter becomes purely a legacy compatibility API for third party code to use that the interpreter itself bypasses.


sys.__interactivehook__ = register_readline

Choose a reason for hiding this comment

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

I think this change to the way enablerlcompleter works is still concerning.

Reviewing 0e6d9e2 I think the public API to preserve would be:

New top level function that actually does the registration is called _register_readline (as @tomviner originally had it)
Reduced enablerlcompleter implementation just does sys.__interactive_hook__ = _register_readline
_set_interactive_hook call _register_readline directly, bypassing enablerlcompleter (since it is setting its own hook)

enablerlcompleter would never be called by the standard library (outside the test suite), it would purely be a backwards compatibility API accounting for the fact that it was never marked as private in the interactive help.

picnixz changed the title~~bpo-28140: Add help message for pip in REPL~~ gh-72327: Add help message for pip in REPL

Dec 15, 2024

This PR is stale because it has been open for 30 days with no activity.