-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
STY: fixes Pylint C0209
Errors (uses f-strings)
#26279
base: main
Are you sure you want to change the base?
Conversation
C0209
Errors (uses f-strings)C0209
Errors (uses f-strings)
I don't have a problem with this update, f strings are easier to read because they are more self documenting. |
C0209
Errors (uses f-strings)C0209
Errors (uses f-strings)
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 started reading this but only got partway through before I ran out of time for this session of code review. I think the original version of this PR that I reviewed before was a lot shorter? It would be easier to review if you broke this up into multiple PRs that all converted a single "pattern" of usage.
Stepping back, the reason people tend to avoid refactors like this is because of the tendency to accidentally introduce bugs in code that might have used outdated idioms but was still working perfectly fine. It's also tedious to review code changes like this, especially if there's so many changes at once. There's a tendency to gloss over and not notice issues.
I would also suggest taking a look at automatic code upgraders like ruff
or pyupgrade
to do this work (I think you did this manually, please correct me if that's wrong). It would make it easier to review if you could just give me a ruff
incantation to apply to get the exact PR diff you're trying to upstream, that way I don't have to worry about a human making a mistake somewhere.
) | ||
positional = ', '.join( | ||
f'out{i+1}' for i in range(ufunc.nout)) | ||
out_args = f'[, {positional}], / [, out={repr((None,)*ufunc.nout)}]' |
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.
nit: binding a default
variable as well would be more readable IMO
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, good point. I'll look into making it more readable.
@@ -539,7 +539,7 @@ def put(a, ind, v, mode='raise'): | |||
put = a.put | |||
except AttributeError as e: | |||
raise TypeError("argument 1 must be numpy.ndarray, " | |||
"not {name}".format(name=type(a).__name__)) from e | |||
f"not {type(a).__name__}") from e |
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.
nit: i'm pretty sure this is how e.g. black would align it
f"not {type(a).__name__}") from e | |
f"not {type(a).__name__}") from e |
Oh also if we're going to make a global code change like this, we should probably add a linter rule to check for this. |
Thanks for taking a look @ngoldbaum. That makes sense. Yes, the original pull request was slightly shorter; I adjusted the Regex expression I was using to find
That makes sense. I thought it could be convenient/cleaner to make these adjustments, but totally understand that they don't actually impact the day-to-day functionality of
Yes, all of these changes were done manually. I took a lot of care to make sure I didn't accidentally change the resulting string, but I can see how this would be difficult to code review and catch potential bugs. I'll look into those automated tools you mentioned.
Yeah, I'll look into this as well and adding a linter rule that can identify these types of things. As mentioned in #26278, the specific linting rule that catches these kinds of issues is Pylint |
I suggest to close this then. Large diff for style-only changes are not desirable - a lot of work to review and the risk of regressions. A new PR with some selected tool-generated changes would be better. That said, please don't overdo it - style-only PRs are in general not the best thing to work on. |
I went through the
numpy
repository and updated all of the strings using'{}'.format()
to be equivalent f-strings, resolving the PylintC0209
linting errors that were prevalent throughout the repository. In terms of the functionality of the code, nothing was changed, but the code has been updated to meet more current Python style standards (see PylintC0209
for more details).Resolves #26278