-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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-129545 Improve Syntax Error Message for Default Parameter Order #130937
base: main
Are you sure you want to change the base?
Conversation
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
note: will add news entry tmw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @sharktide! I'm good with this change. I've left one comment about a formatting issue.
Lib/test/test_positional_only_arg.py
Outdated
check_syntax_error(self, "def f(a, b = 5, /, c): pass", "positional parameter without a default follows parameter with a default ") | ||
check_syntax_error(self, "def f(a = 5, b, /, c): pass", "positional parameter without a default follows parameter with a default ") | ||
check_syntax_error(self, "def f(a = 5, b=1, /, c, *, d=2): pass", "positional parameter without a default follows parameter with a default ") | ||
check_syntax_error(self, "def f(a = 5, b, /): pass", "positional parameter without a default follows parameter with a default ") | ||
check_syntax_error(self, "def f(a, /, b = 5, c): pass", "positional parameter without a default follows parameter with a default ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the additional trailing spaces here. They shouldn't be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lysnikolaou Done ready for re-review/merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lysnikolaou Does a small change need a news entry? I didn't know if to put one or not. Since you commented it looks good other than the formatting, I am assuming not, but if so, could you please apply the "skip news" label?
the bedevere-bot keeps notifying me of a missing news entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sharktide A news entry would be best here, since there's a user-facing change, so please add one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add it under the topic c-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lysnikolaou Added the news entry, everything done
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One editorial comment on the news blurb. Also, a better category for it would be Core and Builtins
. Maybe move it there?
Misc/NEWS.d/next/C_API/2025-03-07-16-48-12.gh-issue-129545.t-9iTH.rst
Outdated
Show resolved
Hide resolved
…iTH.rst Co-authored-by: Lysandros Nikolaou <[email protected]>
ok |
@lysnikolaou I moved the news entry to Core and Builtins as well as took your change suggestion |
Fixes and closes #129545
Overview:
The
SyntaxError: parameter without a default follows parameter with a default
error message has been modified toSyntaxError: positional parameter without a default follows parameter with a default
to clarify error messages.As mentioned in #129545
@smheidrich
As also mentioned by @smheidrich:
The new error message, should be accurate for most cases and should not be confusing. However, if there are cases where the original error message might be more appropriate ex when dealing with keyword-only parameters or other specific syntax rules but the new change would cause minimal confusion compared to the problem that this pr intends to solve.
SyntaxError: parameter without a default follows parameter with a default
is inaccurate #129545