Fix a false positive for class attribute typed with Final #10712
Conversation
Pierre-Sassoulas
added
False Positive π¦
|
Do you have any thoughts on the expected messages Jacob ? |
|
|
||
| @dataclass | ||
| class Class: | ||
| class_snake_case_constant: Final[int] = 42 # [invalid-name] |
There was a problem hiding this comment.
This line should not raise [invalid-name], because Final attributes can still be re-assigned a new value that is different from the default value in __init__(). Final only protects the value after the instance is created.
Final is like const in C/C++. UPPER_CASE is like the marco which gets expanded during static interpretation of the code.
So UPPER_CASE is more immutable than Final.
So marking a dataclass attribute Final is not the sufficient condition for UPPER_CASE.
There was a problem hiding this comment.
Let's discuss that somewhere else. I don't want to make Final more special than it already is. The only thing we grok about Final is ...
| @dataclass | ||
| class Class: | ||
| class_snake_case_constant: Final[int] = 42 # [invalid-name] | ||
| CLASS_UPPER_CASE_CONSTANT: Final[int] = 42 |
There was a problem hiding this comment.
This line should be OK. Stacking them together should not raise any [invalid-name].
There was a problem hiding this comment.
... right here. (I agree with you)
Everything else in this file should be like Final doesn't even exist.
| @dataclass | ||
| class Class: | ||
| class_snake_case_constant: Final[int] = 42 # [invalid-name] | ||
| CLASS_UPPER_CASE_CONSTANT: Final[int] = 42 |
There was a problem hiding this comment.
I suggest adding a just UPPER_CASE attribute test case like:
CLASS_UPPER_CASE_ATTRIBUTE: int = 42This line should not raise [invalid-name] here, because UPPER_CASE can be used alone to identify a static constant.
But later on if it gets re-assigned new value in the below method, then [invalid-name] should be raised there to ask for snake_case because it is no longer a constant.
| CLASS_UPPER_CASE_CONSTANT: Final[int] = 42 | ||
|
|
||
| def method(self) -> None: | ||
| method_snake_case_constant: Final[int] = 42 # [invalid-name] |
There was a problem hiding this comment.
I suggest adding a test case for
self.CLASS_UPPER_CASE_ATTRIBUTE = 36 # [invalid-name]
Because this attribute is named with UPPER_CASE letters, it should not be re-assigned a new value by any means. If it is re-assigned a new value, we can safely raise [invalid-name] on the spot and asking for snake_case names instead.
|
I'll aim to take a look this week at fixing this. |
|
π€ According to the primer, this change has no effect on the checked open source code. π€π This comment was generated for commit 5fc003c |
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10712 +/- ##
=======================================
Coverage 95.97% 95.97%
=======================================
Files 176 176
Lines 19537 19537
=======================================
Hits 18750 18750
Misses 787 787
π New features to boost your workflow:
|
Pierre-Sassoulas
temporarily deployed
to
Backport
GitHub Actions
Inactive
Co-authored-by: Jacob Walls <[email protected]> (cherry picked from commit 133681e)
Type of Changes
Description
Closes #10711