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

Demonstration of old python code using pyupgrade #2100

Closed
wants to merge 4 commits into from

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Aug 10, 2023

This is a minimal run of pyupgrade to bring obsolete code and old standards up to par with Python 3.7. And enforce them through GitHub action. Same idea as #1990, but automated to Python 3.7.
If have turned on all the --keep-percent-format flag and ignored adodbapi to keep changes to a minimum. (other flags had no effect on this codebase)

All python changes in this PR are automated, but here's an overview of the different types of changes seen in this PR:

@@ -135,8 +135,8 @@ def Generate(inpath, outpath, commentPrefix, eolType, *lists):
# print "generate '%s' -> '%s' (comment prefix: %r, eols: %r)"\
# % (inpath, outpath, commentPrefix, eolType)
try:
infile = open(inpath, "r")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know Scintilla is vendored, but these changes are automated. Just like pycln, isort and black, is that fine? I assume the idea is that we trust this won't lead to behaviour changes and that updating Scintilla won't cause these changes to be lost because they'll be automatically re-applied.

Copy link
Owner

Choose a reason for hiding this comment

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

The problem then will be that re-vendoring in the future would break, meaning we'd need to work out what to re-run etc. It seems better long term to just ignore scintilla entirely, which works both today and in the future.

Copy link
Owner

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

This has very marginal value IMO. pyupgrade describes itself as "A tool ... to automatically upgrade syntax for newer versions of the language." but it is clearly going way beyond that - it's also very opinionated about stylistic things and I'm not convinced we need yet another opinionated tool. Are any of these changes actually going to become syntax errors in later Python versions?

@@ -135,8 +135,8 @@ def Generate(inpath, outpath, commentPrefix, eolType, *lists):
# print "generate '%s' -> '%s' (comment prefix: %r, eols: %r)"\
# % (inpath, outpath, commentPrefix, eolType)
try:
infile = open(inpath, "r")
Copy link
Owner

Choose a reason for hiding this comment

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

The problem then will be that re-vendoring in the future would break, meaning we'd need to work out what to re-run etc. It seems better long term to just ignore scintilla entirely, which works both today and in the future.

@@ -1,7 +1,7 @@
"""
Implements a permissions editor for services.
Service can be specified as plain name for local machine,
or as a remote service of the form \\machinename\service
or as a remote service of the form \\machinename\\service
Copy link
Owner

Choose a reason for hiding this comment

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

I'm quite surprised this tool is so opinionated about comments, but it got it wrong here - the existing "\\" should now be "\\\\"

Copy link
Collaborator Author

@Avasam Avasam Aug 17, 2023

Choose a reason for hiding this comment

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

I don't think it's an opinion about comments: it correctly found (and fixed) an invalid escape sequence in the dosctring: \s. It didn't touch the existing \\ because that"s a valid \ character once escaped.

That being said, you're right this docstring should have \\\\machinename\\ to properly escape to \\machinename\ (or make the whole thing a raw docstring)

@Avasam
Copy link
Collaborator Author

Avasam commented Aug 17, 2023

This has very marginal value IMO. […] it's also very opinionated about stylistic things and I'm not convinced we need yet another opinionated tool.

Understandable :) I still think there's a couple things shown in here that are worth considering in isolation. Even if they're not currently enforced by tooling (which could be covered by some more popular and configurable tools like Flake8/Ruff, even autofixed).

Namely:

  • Redundant extra parenthesis (like ((foo))+bar)
  • Use of obsolete / redundant aliases (like OSError aliases EnvironmentError, IOError, WindowsError, socket.error, select.error and mmap.error). They all mean the same thing since Python 3.3. Using arbitrarily different names can be confusing (one might think they are different errors).
  • Use of deprecated aliases
  • Invalid escape sequences
  • More concise and less error-prone, "zero arguments" form of super calls
  • (not shown here and opinionated) more concise f-strings interpolation over "% strings" or .format calls.
  • Use of collections literals and comprehensions (there's some performance gains here, on top of often more concise syntax)

For your consideration to be completed in individual PRs.

@mhammond
Copy link
Owner

Yeah, I don't object to those change, but am less keen on having CI force this tool's opinions. I'm also mildly surprised none of the other tools seem to consider these worth noting?

@Avasam Avasam marked this pull request as draft August 18, 2023 18:17
@Avasam Avasam changed the title Minimal run of pyupgrade (Python 3.5, all keep-* flags) Demonstration of old python code using pyupgrade (Python 3.5, all keep-* flags) Aug 18, 2023
@Avasam
Copy link
Collaborator Author

Avasam commented Aug 18, 2023

I'm also mildly surprised none of the other tools seem to consider these worth noting?

Black is only a formatter, not a linter. isort and pycln only deal with imports. I've already fixed in previous PRs a bunch of issues raised by mypy and pyright. Flake8/Ruff/pylint are the tools that would raise these kind of code smells / potential issues, whilst being configurable, but they are not currently used.

I've updated the title of the PR to reflect this more as a demo, I'll split the changes by their scope.

@Avasam Avasam changed the title Demonstration of old python code using pyupgrade (Python 3.5, all keep-* flags) Demonstration of old python code using pyupgrade Aug 22, 2023
@Avasam
Copy link
Collaborator Author

Avasam commented Sep 21, 2023

@mhammond Concerning Redundant open modes (open("foo", "r") --> open("foo")) I can see preferring explicit open mode (because the default can change across language, so a reader/maintainer's expectation of "default" may differ if they don't explicitly remember python is r only by default) . Thoughts ?


Also, do you have a certain preference for using printf-style formatting? ("%s" %) over .format or f-strings? Because if not:
https://docs.astral.sh/ruff/rules/printf-string-formatting/#why-is-this-bad

printf-style string formatting has a number of quirks, and leads to less readable code than using str.format calls or f-strings. In general, prefer the newer str.format and f-strings constructs over printf-style string formatting.

https://docs.astral.sh/ruff/rules/f-string/#why-is-this-bad

f-strings are more readable and generally preferred over str.format calls.

https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting

Note The formatting operations described here exhibit a variety of quirks that lead to a number of common errors (such as failing to display tuples and dictionaries correctly). Using the newer formatted string literals, the str.format() interface, or template strings may help avoid these errors. Each of these alternatives provides their own trade-offs and benefits of simplicity, flexibility, and/or extensibility.

@mhammond
Copy link
Owner

@mhammond Concerning Redundant open modes (open("foo", "r") --> open("foo")) I can see preferring explicit open mode (because the default can change across language, so a reader/maintainer's expectation of "default" may differ if they don't explicitly remember python is r only by default) . Thoughts ?

I don't feel strongly about this, although I am perfectly fine with no mode arg - https://docs.python.org/3/tutorial/inputoutput.html explicitly says "The mode argument is optional; 'r' will be assumed if it’s omitted."

Also, do you have a certain preference for using printf-style formatting? ("%s" %) over .format or f-strings? Because if not: https://docs.astral.sh/ruff/rules/printf-string-formatting/#why-is-this-bad

Now we can assume they exist, I prefer f-strings. However, I'm not sure I prefer them enough to bother touching every string format in the tree.

@Avasam
Copy link
Collaborator Author

Avasam commented Sep 21, 2023

All changes categories have been extracted into their own PRs. They could be configured and enforced using Ruff, and automated using pre-commit.ci (#2034). I will close this as conversation about using modern standards can be moved to those PRs.

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.

2 participants