-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add consider-using-augmented-assign
checker
#7514
Conversation
Something is up with the caches of the primer. Seems to have to do with a bump in the Python versions used by the runner that is happening in between the creation of the cache and the calling to the cache. Let's see if all things return back to normal after the run on |
Pull Request Test Coverage Report for Build 3107823195
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
Noticed the first Django result is a false positive (string formatting, not modulo): |
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.
Nice! And quite a bit more robust than the initial implementation 🚀
tests/functional/c/consider/consider_using_augmented_assign.txt
Outdated
Show resolved
Hide resolved
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.
Requesting a test showing that %=
does not raise a false positive.
Thanks for checking! PRs like these are where the primer shines 😄 |
Can't just simplify that as well though? ❯ python
Python 3.10.0 (default, Oct 27 2021, 16:20:25) [Clang 13.0.0 (clang-1300.0.29.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> x = "%s"
>>> x % "my_string"
'my_string'
>>> x %= "my_string"
>>> x
'my_string' |
Ah, that might not be a false positive then. Still I'm worried there could be something lurking with incomplete string formatting (e.g. |
I think those would be an issue without the augmented assign as well, but you're right: better safe than sorry! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I don't understand I think this is ready for review! |
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.
There's a badly formatted message for black; Use '=' to do an augmented assign directly*
(https://github.com/psf/black/blob/6ae8457a8628345c59fed22c58728ffa258a3e76/src/blib2to3/pgen2/literals.py#L51). It also happen for pandas here : https://github.com/pandas-dev/pandas/blob/71fc89cd515c3c19230fbab64e979118858b808a/pandas/tests/io/formats/test_format.py#L1535, so probably for all *
.
I added a suggestion for some functional test with operators I could think of.
Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com>
This is incorrect formatting from GitHub. See test output for correct examples with |
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.
Great idea for a checker, and nice execution 👌
I haven't followed the discussion around this check so it might have come up already. Why was this not implemented as an optional extension? From what I can tell there is no obvious benefit to using the augmented assignment over the regular one. IMO it just creates a lot of unnecessary noise. Similar checks like the one for assignment expressions are also optional. |
I think @cdce8p has a point here. |
Agreed. |
Agreed. I'll create a PR when I get the time. |
See opened issue for follow up question. |
towncrier create <IssueNumber>.<type>
which will beincluded in the changelog.
<type>
can be one of: new_check, removed_check, extension,false_positive, false_negative, bugfix, other, internal. If necessary you can write
details or offer examples on how the new change is supposed to work.
and preferred name in
script/.contributors_aliases.json
Type of Changes
Description
Closes #3391
Based around the work by @clavedeluna in #7509. But to make this checker work I had to add some additional checking.
Let's see what the primer thinks of this! 😰