[flake8-naming]: Respect import conventions (N817)#12922
Conversation
| | ^^^^^^^^^^^^^^^ N817 | ||
| | | ||
|
|
||
| N817.py:6:8: N817 CamelCase `ElementTree` imported as acronym `ET` |
There was a problem hiding this comment.
The configuration for this test run specifies that the convention for importing ElementTree is XET. That's why ET gets flagged.
There was a problem hiding this comment.
And the pre-existing test ensures that import ElementTree as ET is not flagged with the default settings, correct?
600c1f2 to
173cc67
Compare
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| N817 | 2 | 0 | 2 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+0 -2 violations, +0 -0 fixes in 1 projects; 53 projects unchanged)
apache/airflow (+0 -2 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
- scripts/ci/testing/summarize_junit_failures.py:23:8: N817 CamelCase `ElementTree` imported as acronym `ET` - scripts/in_container/check_junitxml_result.py:21:8: N817 CamelCase `ElementTree` imported as acronym `ET`
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| N817 | 2 | 0 | 2 | 0 | 0 |
AlexWaygood
left a comment
There was a problem hiding this comment.
LGTM. I think this is what users would intuitively expect and is a reduction in scope, so I don't see it as particularly breaking
| /// [`lint.flake8-boolean-trap.extend-allowed-calls`] option are allowed. | ||
| /// | ||
| /// |
There was a problem hiding this comment.
| /// [`lint.flake8-boolean-trap.extend-allowed-calls`] option are allowed. | |
| /// | |
| /// | |
| /// [`lint.flake8-import-conventions.banned-aliases`] option are allowed. | |
| /// |
Also, I notice you used square brackets but didn't add a link at the end. Was that deliberate -- do we have a mechanism that adds links automatically to settings?
There was a problem hiding this comment.
Yes, we have some fancy script that resolves settings (and validates them)
There was a problem hiding this comment.
Fancy script or no fancy script, you're still referring to the wrong setting here: the lint.flake8-boolean-trap.extend-allowed-calls setting has nothing to do with this rule
| | ^^^^^^^^^^^^^^^ N817 | ||
| | | ||
|
|
||
| N817.py:6:8: N817 CamelCase `ElementTree` imported as acronym `ET` |
There was a problem hiding this comment.
And the pre-existing test ensures that import ElementTree as ET is not flagged with the default settings, correct?
flake8-naming]: Respect import conventions (N8171)flake8-naming]: Respect import conventions (N817)
Summary
Fixes #12916 by respecting the configured import conventions in
N817.I decided to respect the conventions regardless on whether
ICN001is enabled or not because it makes the change easier to explain (fewer conditions when the behavior applies)and seems like a reasonable default.
Test Plan
Added tests
Breaking change
One could argue that this is a breaking change because
ETimports flagged before will now no-longer be flagged, requiring users to adopt their configuration if they want to keep flaggingET.I think it's fine but interested in more opinions.