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

gh-121804: always show error location for SyntaxError's in new repl #121886

Merged
merged 7 commits into from
Aug 19, 2024

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Jul 17, 2024

>>> def good(x, y): ...
... def bad(x, x): ...
  File "<python-input-13>", line 2
    def bad(x, x): ...
               ^
SyntaxError: duplicate argument 'x' in function definition

Notes:

  • showsyntaxerror() has undocumented colorize kwarg; new source argument also left undocumented
  • similar fix is possible for basic repl (see issue thread, here); if that does make sense - I'll provide a separate pr

…repl

>>> def good(x, y): ...
... def bad(x, x): ...
  File "<python-input-13>", line 2
    def bad(x, x): ...
               ^
SyntaxError: duplicate argument 'x' in function definition
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

I presume that this will be backported to 3.13. I would like the pyshell addition backported to 3.12 since I showsyntaxerror needs revision. If you don't, I will.

@skirpichev
Copy link
Member Author

I presume that this will be backported to 3.13.

I'm not sure that referenced issue is important enough to do backport. The #121804 is not specific for the new repl. It's valid for the basic repl as well, long before 3.12.

I would like the pyshell addition backported to 3.12 since I showsyntaxerror needs revision. If you don't, I will.

I doubt it does make sense. Support for kwargs was silently added for showsyntaxerror() in #119318, it's not backported to 3.12. But in 3.13+, probably subclasses of InteractiveInterpreter should be adjusted to accept kwargs.

This patch doesn't fix anything for IDLE, apart from this small incompatibility in interfaces. The referenced issue isn't valid for the IDLE - here always correct error locations are highlighted.

@skirpichev skirpichev requested a review from terryjreedy July 31, 2024 05:51
Lib/code.py Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
Add new kwarg ``source`` for
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to make this argument public so let's not mention it in the change log . Instead, mention what we are fixing in the repl

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I should make it underscore-prefixed?

Copy link
Member

Choose a reason for hiding this comment

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

Our experience is that if it appears in the signature, people will use it and then changing it will be more challenging. Also some times the underscore prefixed argument is used to avoid collision with keywords or other reserved identifiers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Our experience is that if it appears in the signature, people will use it and then changing it will be more challenging.

Yeah, I did a proposal to hide this per default: https://discuss.python.org/t/25543.

Also some times the underscore prefixed argument is used to avoid collision with keywords

I was thinking that it's pep8 violation. "it is generally better to append a single trailing underscore rather than use an abbreviation or spelling corruption." (c)

@skirpichev skirpichev requested a review from pablogsal August 19, 2024 12:56
@pablogsal pablogsal added the needs backport to 3.13 bugs and security fixes label Aug 19, 2024
@pablogsal pablogsal merged commit 354d55e into python:main Aug 19, 2024
45 checks passed
@miss-islington-app
Copy link

Thanks @skirpichev for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @skirpichev and @pablogsal, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 354d55eb1fa40f272419aa6459ee5d2c4804c8ea 3.13

@pablogsal
Copy link
Member

Thanks a lot for your contribution @skirpichev 🚀

@pablogsal
Copy link
Member

Sorry, @skirpichev and @pablogsal, I could not cleanly backport this to 3.13 due to a conflict. Please backport using cherry_picker on command line.

cherry_picker 354d55eb1fa40f272419aa6459ee5d2c4804c8ea 3.13

Seems the backport failed, do you mind following these instructions to open the 3.13 backport?

@skirpichev
Copy link
Member Author

skirpichev commented Aug 19, 2024 via email

@skirpichev skirpichev deleted the show-syntaxerr-location-121804 branch August 19, 2024 15:45
skirpichev added a commit to skirpichev/cpython that referenced this pull request Aug 19, 2024
…in new repl (pythonGH-121886)

(cherry picked from commit 354d55e)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Aug 19, 2024

GH-123148 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Aug 19, 2024
pablogsal pushed a commit that referenced this pull request Aug 19, 2024
jeremyhylton pushed a commit to jeremyhylton/cpython that referenced this pull request Aug 19, 2024
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
@bedevere-app
Copy link

bedevere-app bot commented Aug 26, 2024

GH-123366 is a backport of this pull request to the 3.12 branch.

terryjreedy added a commit that referenced this pull request Aug 26, 2024
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