N811 & N814: eliminate false positives for single-letter names#14584
N811 & N814: eliminate false positives for single-letter names#14584AlexWaygood merged 11 commits intoastral-sh:mainfrom
Conversation
|
There was a problem hiding this comment.
Thanks.
I'm trying to understand the change in regards of PEP8 but I wasn't able to find the exception for single-letter non-constants. Could you extend the rule's documentation and mention the exception and reference PEP8. It will help to prevent future issues that ask for Ruff detecting single-letter uppercase imports to be flagged.
crates/ruff_linter/src/rules/pep8_naming/rules/constant_imported_as_non_constant.rs
Outdated
Show resolved
Hide resolved
snowdrop4
force-pushed
the
AVK/ConstantImportedAsNonConstant
branch
from
22e8b0a to
3da4c28
Compare
|
PEP8 doesn't explicitly mention the case. It leaves it undefined by failing to mention the obvious problem. It only says:
If there is only one character then CapWords and ALL_CAPS_SNAKE_CASE appear the same since the first character is always capitalised. But if there are at least two characters, then 99.99% of the time, it isn't ambigious any more, since it would either be But yeah, good point about the docs. I've added a |
There was a problem hiding this comment.
Thanks. This makes sense to me. @AlexWaygood it would be great if you could do a quick sanity check if the rule-change makes sense to you too.
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Some docs nits. We refer to it elsewhere in our docs as CamelCase rather than PascalCase.
crates/ruff_linter/src/rules/pep8_naming/rules/camelcase_imported_as_constant.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pep8_naming/rules/camelcase_imported_as_constant.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pep8_naming/rules/constant_imported_as_non_constant.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pep8_naming/rules/constant_imported_as_non_constant.rs
Outdated
Show resolved
Hide resolved
| // Single-character names are ambiguous. It could be a class or a constant. | ||
| name.chars().nth(1)?; |
There was a problem hiding this comment.
But we still want to forbid things like from foo import C as c from one of these rules, right? Won't these changes mean that we will no longer see that error?
There was a problem hiding this comment.
It's not possible to tell if C is a class or a constant. So even though from foo import C as c might not be advisable, it's not applicable to throw an error about this under a rule made for constants.
There was a problem hiding this comment.
But regardless of whether you classify its original name as CamelCase or SCREAMING_SNAKE_CASE, the name it's being aliased to is neither CamelCase nor SCREAMING_SNAKE_CASE. I don't mind much which of these two rules flags that, but I think I would want a rule in this category to complain that the alias uses a different naming convention to the naming convention used for the object's original name.
There was a problem hiding this comment.
I could add a new rule, non_lowercase_imported_as_lowercase? (to compliment the already-existing lowercase_imported_as_non_lowercase).
Though I guess we'd need to prevent overlaps and check that the other rules aren't already firing?
Or maybe just name it single_uppercase_character_imported_as_lowercase?
There was a problem hiding this comment.
Would it be possible to preserve the existing behavior for lower-case letters which is that both rules falg it?
Nice catch @AlexWaygood !
There was a problem hiding this comment.
Would it be possible to preserve the existing behavior for lower-case letters which is that both rules falg it?
Yeah, that could be an option.
There was a problem hiding this comment.
I've just pushed a change that makes N811 flags it. I think that rule makes more sense here, and I think it's not a great outcome if one lint error is flagged by two rules. I don't think the situation is common enough to justify its own rule.
There was a problem hiding this comment.
a great outcome if one lint error is flagged by two rules
I don't disagree with this. I'm just slightly worried about bug reports if someone only enables one rule... But I'm fine with either
AlexWaygood
force-pushed
the
AVK/ConstantImportedAsNonConstant
branch
from
8d59bf8 to
7ae363c
Compare
|
Thanks @snowdrop4! |
|
No problem. Thanks for the reviews! |
3 participants
Summary
Under PEP8, identifiers consisting of a single upper-case character could either be a class or a constant.
N811 (constant imported as non-constant) and N814 (camelcase imported as constant) are, however, incorrectly assuming these identifiers to always be constants, which leads to false-positives.
Although identifiers consisting of a single upper-case character are not advised under PEP8, it would be best if these cases were handled by more specific rule(s) that point out ambiguous identifiers and/or violations of PEP8, rather than rules that attempt to narrowly point out changes in identifier casing.
Addresses #11862.
Test Plan
Additional fixture cases.