Apply variable name regex to module-level names that are reassigned #3585 #10212
Conversation
jacobtylerwalls
added
the
False Positive π¦
jacobtylerwalls
added
False Negative π¦
This comment has been minimized.
This comment has been minimized.
jacobtylerwalls
force-pushed
the
invalid-name-module
branch
from
46fe6c6 to
91c399d
Compare
This comment has been minimized.
This comment has been minimized.
Codecov ReportAll modified and coverable lines are covered by tests β
Additional details and impacted files@@ Coverage Diff @@
## main #10212 +/- ##
==========================================
+ Coverage 95.84% 95.86% +0.02%
==========================================
Files 175 175
Lines 19056 19069 +13
==========================================
+ Hits 18264 18281 +17
+ Misses 792 788 -4
π New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
This is a great PR ! Deal with one of the major issue with pylint (after the under 3 letters issue that you already tackled before) and greatly improves it.
I'm wondering if we should disable invalid-name on all tests unrelated to invalid-name instead of renaming variables in order to reduce churn ?
| @@ -0,0 +1,7 @@ | |||
| `invalid-name` now distinguishes module-level names that are assigned only once | |||
There was a problem hiding this comment.
This should probably be advertised as a breaking change for 4.0, it make sense to do it this way but pylint have been doing it a certain way for so long that "module level as constant" became a convention of a kind. (LOGGER vs logger was not settled last time I checked, admittedly a long time ago).
I wondered about that also, and I'm open to it (would make the review easier), but it would mean that future changes would not notice regressions in these files. I did find cases that were important to handle by going through these files (functions, lambdas, union types, type vars), and a future contributor might find that just as useful if making changes next time someone asks us to adjust this behavior. WDYT? |
|
Let's not revert then. Another way to 'fix' this would be to authorize both uppercase and snake case for module level variable but not something inconsistant like _UPPER_CamelCase (as seen in the primer). I guess it's a cope out but it does have the advantages to never have false positives. Also if you had a logger and you add a formater to it you won't suddenly have to change the variable name to snake_case. Maybe we could let the user choose to make pylint 'dumber' ? What do you think ? |
|
Right, that's option 2 discussed a few times on the issue. I'm not sure it's worth the complexity. Too many settings, harder to reason about and customize. Let's just proselytize for |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| inferred_left = safe_infer(binop.left) | ||
| if isinstance(binop.left, nodes.Const): | ||
| # This bizarrely became necessary after an unrelated call to igetattr(). | ||
| # Seems like a code smell uncovered in #10212. |
There was a problem hiding this comment.
Not sure if I understand what the code smell is here, would you mind explaining a little more ?
There was a problem hiding this comment.
The fact that I called igetattr() in my PR seemed to cause a cache-creation or cache-eviction side effect somewhere someplace causing a test to fail. It failed in a way where binop.left was already a Const and didn't need to be inferred, so I just handled for it. Debugging the cached side effect sounds like a nightmare, I didn't get that far.
I'd leave a more helpful comment if I could think of one, I'm definitely open to suggestions! Wanted to at least leave some kind of breadcrumb for the next developer.
| ITEM_LIST.append(item) | ||
| ITEM_LIST.remove(item) |
There was a problem hiding this comment.
We're modifying a supposed constant here ? Look like a false negative ?
There was a problem hiding this comment.
It's still a constant even if it's mutable, right?
There was a problem hiding this comment.
I think constants should be immutable but that might be an issue with the definition of what a constant is supposed to be in python (Time to read some PEP I guess?). I think I remember some argument by a user that this message will always have false positives / false negatives because it's "impossible" to know if a python object is an immutable or not. I kinda like the approach of determining if something is a constant if it's reassigned or muted in the actual code regardless of the actual immutable/mutable property of the instance we're analyzing.
There was a problem hiding this comment.
It's a judgment call for sure, but it strikes me as a little fancy to do that much checking. A list that gets appended to a couple times depending on a couple of if conditions at the top of a module is still a constant when another module imports it.
I'm open to other views, so no rush here, but I don't foresee this being much of a problem in practice (other than needing to silence invalid name in situations similar to: in a Django settings.py file (assuming it's even linted):
if DEBUG:
SILENCED_SYSTEM_CHECKS += [...]There was a problem hiding this comment.
Hmm, now that you say it, I definitely mutate constants in django setting's files. It think I remember django rosetta suggesting this at some point:
if DEBUG:
INSTALLED_APPS.append("rosetta")It would be annoying to silence this all the time. (Maybe something for pylint-django to take care off if we choose to go this way.) Let's wait for other opinions to do it. But in any case this MR is already a huge improvement, we could merge as is (and create a discussion issue to be resolved before we release 4.0?).
There was a problem hiding this comment.
I was arguing that constants shouldn't be mutated either, if we do the check so that "mutated variable are not constant so they should be snake case" then it would start to be a problem in django settings too.
There was a problem hiding this comment.
if we do the check so that "mutated variable are not constant so they should be snake case"
Maybe I'm missing what you're saying - I would be against such a check, since I think a mutated constant is still a constant.
There was a problem hiding this comment.
Yes we disagree on this. PEP8 is very vague about this, there's no definition of what a constant is. After searching for ~= 15mn I didn't find anything authoritative to support or refute my opinion in the PEP, so I welcome opinion from anyone reading this.
Imo a constant value should not change during the execution of the program. Mutating means it's modified during execution so it contradicts the idea of being a constant. (In the case of django the settings.py is read/executed once for each server launch so it's not wrong to call it a constant after the settings.py is executed and the server is launched).
There was a problem hiding this comment.
Personally I would be cautious about mutating constants in runtime like what @Pierre-Sassoulas has mentioned, but there seems to be actual use cases as mentioned above (in Django settings).
Since there is no official definition in PEP, I think it all boils down to how strictly we want to treat constants, it could go both ways really. @jacobtylerwalls is probably thinking about constants like how the const keyword works in Javascript? Where a constant declaration cannot be reassigned but can be mutated. At the same time, we have languages like CPP that take it one step further by enforcing immutability on constant objects.
There was a problem hiding this comment.
Yeah, the const keyword in javascript is exactly how I'm thinking about it. Plus, python explicitly has no concept of immutable objects, so "constant" is really more of a shorthand. Maybe it's unfortunate we're discussing the word "constant" at all. That's why I'm preferring to speak about "reassignments".
|
/cc @DanielNoord if you have thoughts. We have to take a decision on the two paths in #3585 (comment): either allow module-level names to pass against either of two regexes (const or variable), or do some static analysis to determine which regex to use. This PR currently does static analysis to determine which regex to use. It seemed from the discussion (to me) that folks were in agreement that a little static analysis would be fine. I'm proposing to only analyze reassignments, sidestepping the whole question of "what is a constant". (Perhaps our "constant" name regex is just unfortunately named!) Pierre has some concerns about only looking at reassignments and not mutations, I think. |
|
This MR makes pylint better and we can merge as is, not favoring false negative on this check is a pre-existing issue. Making this check better might push users toward using this check after reading the changelog and encounter the pre-existing issue though. (Then users asked to uppercase mutated constants would later think "ho no, not again !" like some short-lived petunia pots if asked to snake case them again.). Adding logger/LOGGER to Maybe it's possible to allow both conventions when in doubt, and only snake case on reassignment so we at least don't have anyone moving from snake case to uppercase and back again when we decide later. That way mutation on constant is a false negative not a false positive whatever a particular user's opinion on it is and we don't throw away the great static analysis from this MR. And we can be more opinionated on mutated constant later on when the discussion is resolved. |
|
That's brilliant, that combines both of the approaches sketched on the ticket. It was not as hard as I feared. |
β¦ariable regexes
jacobtylerwalls
force-pushed
the
invalid-name-module
branch
from
cbb41d4 to
2938d11
Compare
This comment has been minimized.
This comment has been minimized.
3 participants
Type of Changes
Description
Closes #3585
Closes #7383
invalid-namenow distinguishes module-level names that are assigned only oncefrom those that are reassigned and now applies
--variable-rgxto the latter.Also,
invalid-nameis triggered for module-level names for additional types(e.g. lists and sets).