Demonstration of old python code using pyupgrade#2100
Demonstration of old python code using pyupgrade#2100Avasam wants to merge 4 commits intomhammond:mainfrom
Conversation
| # print "generate '%s' -> '%s' (comment prefix: %r, eols: %r)"\ | ||
| # % (inpath, outpath, commentPrefix, eolType) | ||
| try: | ||
| infile = open(inpath, "r") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
mhammond
left a comment
There was a problem hiding this comment.
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?
| # print "generate '%s' -> '%s' (comment prefix: %r, eols: %r)"\ | ||
| # % (inpath, outpath, commentPrefix, eolType) | ||
| try: | ||
| infile = open(inpath, "r") |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
I'm quite surprised this tool is so opinionated about comments, but it got it wrong here - the existing "\\" should now be "\\\\"
There was a problem hiding this comment.
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)
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:
For your consideration to be completed in individual PRs. |
|
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? |
keep-* flags)keep-* flags)
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. |
keep-* flags)|
@mhammond Concerning Redundant open modes ( Also, do you have a certain preference for using printf-style formatting? (
https://docs.astral.sh/ruff/rules/f-string/#why-is-this-bad
https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting
|
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."
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. |
|
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. |
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-formatflag 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:
https://github.com/asottile/pyupgrade#format-specifiers + https://github.com/asottile/pyupgrade#printf-style-string-formatting + https://github.com/asottile/pyupgrade#f-strings--> Prefer f-strings and non-printf-style format #2122https://github.com/asottile/pyupgrade#redundant-open-modes--> Redundant open modes #2121https://github.com/asottile/pyupgrade#unittest-deprecated-aliases--> adodbapi: Remove obsolete aliases #2088https://github.com/asottile/pyupgrade#oserror-aliases--> Update and standardise obsoleteOSErroraliases: #2107 & adodbapi: Remove obsolete aliases #2088https://github.com/asottile/pyupgrade#invalid-escape-sequences--> Fix invalid and accidental escape sequences #2112https://github.com/asottile/pyupgrade#super-calls--> Use modern "zero arguments" style forsuper#2106extraneous parens--> Remove extraneous parentheses found by Ruff/pyupgrade #2110https://github.com/asottile/pyupgrade#set-literals--> Update collection literals and comprehensions #2108 & adodbapi: Update collections literals and comprehensions #2109