-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
gh-138281: Run ruff
on Tools/peg_generator
#138282
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
Conversation
You might change the Python minimum version and remove topsort() in a separated change. |
@vstinner done! I removed all semantic changes, now there are only |
Tools/peg_generator/.ruff.toml
Outdated
"UP011", | ||
# Use format specifiers instead of %-style formatting. | ||
# Doesn't always make code more readable. | ||
"UP031", | ||
# Use f-strings instead of format specifiers. | ||
# Doesn't always make code more readable. |
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.
Fine to ignore these.
If you like, you could temporarily enable the locally, do a test run and see if it does make any of them more readable, and just commit those changes?
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 don't think we have a lot of cases where not using f-strings is worse, but checking won't hurt indeed 👍
I am happy if most of the changes are an improvement in any case :)
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.
Yeah, there were two cases, where f
string is better :)
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.
LGTM
@@ -52,7 +53,7 @@ def main() -> None: | |||
try: | |||
grammar, parser, tokenizer = build_parser(args.filename) | |||
except Exception as err: | |||
print("ERROR: Failed to parse grammar file", file=sys.stderr) | |||
print("ERROR: Failed to parse grammar file", err, file=sys.stderr) |
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.
This change should be backported to stable Python versions.
Co-authored-by: Adam Turner <[email protected]>
Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
Sorry, @sobolevn, I could not cleanly backport this to
|
Sorry, @sobolevn, I could not cleanly backport this to
|
I will handle backports. Thanks everyone! |
…-138282) (cherry picked from commit 0d1f4e1) Co-authored-by: sobolevn <[email protected]> Co-authored-by: Adam Turner <[email protected]>
GH-138469 is a backport of this pull request to the 3.14 branch. |
…-138282) (cherry picked from commit 0d1f4e1) Co-authored-by: sobolevn <[email protected]> Co-authored-by: Adam Turner <[email protected]>
GH-138472 is a backport of this pull request to the 3.13 branch. |
Please, note this change:
Technically we did not test python3.8 for a long time and used
python_version = 3.10
for mypy, so I am pretty sure that 3.8 was not supported already.I also removed
topsort
which was not used.ruff
onpeg_generator
#138281