STY: fixes Pylint C0209 Errors (uses f-strings)
#26279
Conversation
HenryAsa
changed the title
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. |
rgommers
changed the title
C0209 Errors (uses f-strings)C0209 Errors (uses f-strings)
There was a problem hiding this comment.
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.
nit: binding a default variable as well would be more readable IMO
There was a problem hiding this comment.
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.
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
numpyrepository and updated all of the strings using'{}'.format()to be equivalent f-strings, resolving the PylintC0209linting 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 PylintC0209for more details).Resolves #26278