Skip to content
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.

Sign up for GitHub

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

Jump to bottom

N811 & N814: eliminate false positives for single-letter names #14584

Merged
merged 11 commits into from
Nov 27, 2024
+72 −4
Merged

N811 & N814: eliminate false positives for single-letter names #14584

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3bd7fd2
reduce false positives with single-letter names
snowdrop4 Nov 25, 2024
3da4c28
don't rely on compiler optimisation
snowdrop4 Nov 25, 2024
b56d933
add one more test case
snowdrop4 Nov 25, 2024
9396f38
add documentation about the ambiguity
snowdrop4 Nov 25, 2024
c811971
Clippy and typo
MichaReiser Nov 26, 2024
1732d6b
remove comments from test files
snowdrop4 Nov 27, 2024
cfc70bb
change ordering of test file
snowdrop4 Nov 27, 2024
3de5771
docs
snowdrop4 Nov 27, 2024
d261a0e
new snaps
snowdrop4 Nov 27, 2024
e9af7fa
backticks in docs to appease clippy
snowdrop4 Nov 27, 2024
7ae363c
Ensure N811 flags single-character constants imported as lowercase
AlexWaygood Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pep8_naming/N811.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
# Error
import mod.CON as c
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
import mod.CONST as const
from mod import CONSTANT as constant
from mod import ANOTHER_CONSTANT as another_constant

# Ok
import django.db.models.Q as Query1
from django.db.models import Q as Query2
5 changes: 5 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pep8_naming/N814.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
# Error
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
import mod.Camel as CAMEL
from mod import CamelCase as CAMELCASE
from mod import AnotherCamelCase as ANOTHER_CAMELCASE

# Ok
import mod.AppleFruit as A
from mod import BananaFruit as B
17 changes: 17 additions & 0 deletions crates/ruff_linter/src/rules/pep8_naming/rules/camelcase_imported_as_constant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,20 @@ use crate::rules::pep8_naming::settings::IgnoreNames;
/// from example import MyClassName
/// ```
///
/// ## Note
/// Identifiers consisting of a single uppercase character are ambiguous under
/// the rules of PEP8, which specifies PascalCase for classes and
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
/// ALL_CAPS_SNAKE_CASE for constants. Without a second character, it is not
/// possible to reliably guess whether the identifier is intended to be part
/// of a PascalCase string for a class or an ALL_CAPS_SNAKE_CASE string for
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
/// a constant, since both conventions will produce the same output when given
/// a single input character. Therefore, this lint rule does not apply to cases
/// where the alias for the imported identifier consists of a single uppercase
/// character.
///
/// A common example of a single uppercase character being used for a class
/// name can be found in Django's `django.db.models.Q` class.
///
/// [PEP 8]: https://peps.python.org/pep-0008/
#[violation]
pub struct CamelcaseImportedAsConstant {
Expand All @@ -53,6 +67,9 @@ pub(crate) fn camelcase_imported_as_constant(
stmt: &Stmt,
ignore_names: &IgnoreNames,
) -> Option<Diagnostic> {
// Single-character names are ambiguous. It could be a class or a constant.
asname.chars().nth(1)?;

if helpers::is_camelcase(name)
&& !str::is_cased_lowercase(asname)
&& str::is_cased_uppercase(asname)
Expand Down
16 changes: 16 additions & 0 deletions crates/ruff_linter/src/rules/pep8_naming/rules/constant_imported_as_non_constant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,19 @@ use crate::rules::pep8_naming::settings::IgnoreNames;
/// from example import CONSTANT_VALUE
/// ```
///
/// ## Note
/// Identifiers consisting of a single uppercase character are ambiguous under
/// the rules of PEP8, which specifies PascalCase for classes and
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
/// ALL_CAPS_SNAKE_CASE for constants. Without a second character, it is not
/// possible to reliably guess whether the identifier is intended to be part
/// of a PascalCase string for a class or an ALL_CAPS_SNAKE_CASE string for
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
/// a constant, since both conventions will produce the same output when given
/// a single input character. Therefore, this lint rule does not apply to cases
/// where the imported identifier consists of a single uppercase character.
///
/// A common example of a single uppercase character being used for a class
/// name can be found in Django's `django.db.models.Q` class.
///
/// [PEP 8]: https://peps.python.org/pep-0008/
#[violation]
pub struct ConstantImportedAsNonConstant {
Expand All @@ -52,6 +65,9 @@ pub(crate) fn constant_imported_as_non_constant(
stmt: &Stmt,
ignore_names: &IgnoreNames,
) -> Option<Diagnostic> {
// Single-character names are ambiguous. It could be a class or a constant.
name.chars().nth(1)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@snowdrop4 snowdrop4 Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@snowdrop4 snowdrop4 Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to preserve the existing behavior for lower-case letters which is that both rules falg it?

Nice catch @AlexWaygood !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if str::is_cased_uppercase(name) && !str::is_cased_uppercase(asname) {
// Ignore any explicitly-allowed names.
if ignore_names.matches(name) || ignore_names.matches(asname) {
Expand Down
38 changes: 26 additions & 12 deletions ...src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__N811_N811.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,40 @@
source: crates/ruff_linter/src/rules/pep8_naming/mod.rs
snapshot_kind: text
---
N811.py:1:8: N811 Constant `CONST` imported as non-constant `const`
N811.py:2:8: N811 Constant `CON` imported as non-constant `c`
|
1 | import mod.CONST as const
1 | # Error
2 | import mod.CON as c
| ^^^^^^^^^^^^ N811
3 | import mod.CONST as const
4 | from mod import CONSTANT as constant
|

N811.py:3:8: N811 Constant `CONST` imported as non-constant `const`
|
1 | # Error
2 | import mod.CON as c
3 | import mod.CONST as const
| ^^^^^^^^^^^^^^^^^^ N811
2 | from mod import CONSTANT as constant
3 | from mod import ANOTHER_CONSTANT as another_constant
4 | from mod import CONSTANT as constant
5 | from mod import ANOTHER_CONSTANT as another_constant
|

N811.py:2:17: N811 Constant `CONSTANT` imported as non-constant `constant`
N811.py:4:17: N811 Constant `CONSTANT` imported as non-constant `constant`
|
1 | import mod.CONST as const
2 | from mod import CONSTANT as constant
2 | import mod.CON as c
3 | import mod.CONST as const
4 | from mod import CONSTANT as constant
| ^^^^^^^^^^^^^^^^^^^^ N811
3 | from mod import ANOTHER_CONSTANT as another_constant
5 | from mod import ANOTHER_CONSTANT as another_constant
|

N811.py:3:17: N811 Constant `ANOTHER_CONSTANT` imported as non-constant `another_constant`
N811.py:5:17: N811 Constant `ANOTHER_CONSTANT` imported as non-constant `another_constant`
|
1 | import mod.CONST as const
2 | from mod import CONSTANT as constant
3 | from mod import ANOTHER_CONSTANT as another_constant
3 | import mod.CONST as const
4 | from mod import CONSTANT as constant
5 | from mod import ANOTHER_CONSTANT as another_constant
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ N811
6 |
7 | # Ok
|
28 changes: 16 additions & 12 deletions ...src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__N814_N814.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,30 @@
source: crates/ruff_linter/src/rules/pep8_naming/mod.rs
snapshot_kind: text
---
N814.py:1:8: N814 Camelcase `Camel` imported as constant `CAMEL`
N814.py:2:8: N814 Camelcase `Camel` imported as constant `CAMEL`
|
1 | import mod.Camel as CAMEL
1 | # Error
2 | import mod.Camel as CAMEL
| ^^^^^^^^^^^^^^^^^^ N814
2 | from mod import CamelCase as CAMELCASE
3 | from mod import AnotherCamelCase as ANOTHER_CAMELCASE
3 | from mod import CamelCase as CAMELCASE
4 | from mod import AnotherCamelCase as ANOTHER_CAMELCASE
|

N814.py:2:17: N814 Camelcase `CamelCase` imported as constant `CAMELCASE`
N814.py:3:17: N814 Camelcase `CamelCase` imported as constant `CAMELCASE`
|
1 | import mod.Camel as CAMEL
2 | from mod import CamelCase as CAMELCASE
1 | # Error
2 | import mod.Camel as CAMEL
3 | from mod import CamelCase as CAMELCASE
| ^^^^^^^^^^^^^^^^^^^^^^ N814
3 | from mod import AnotherCamelCase as ANOTHER_CAMELCASE
4 | from mod import AnotherCamelCase as ANOTHER_CAMELCASE
|

N814.py:3:17: N814 Camelcase `AnotherCamelCase` imported as constant `ANOTHER_CAMELCASE`
N814.py:4:17: N814 Camelcase `AnotherCamelCase` imported as constant `ANOTHER_CAMELCASE`
|
1 | import mod.Camel as CAMEL
2 | from mod import CamelCase as CAMELCASE
3 | from mod import AnotherCamelCase as ANOTHER_CAMELCASE
2 | import mod.Camel as CAMEL
3 | from mod import CamelCase as CAMELCASE
4 | from mod import AnotherCamelCase as ANOTHER_CAMELCASE
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ N814
5 |
6 | # Ok
|
Loading