[airflow] Add autofix infrastructure to AIR302 name checks
#16965
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is intentionally to be kept simple for easier review. I can update the rest nams in this or the following PR after we agree on how this should work. Thanks! |
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| S603 | 77 | 0 | 77 | 0 | 0 |
| RUF100 | 3 | 3 | 0 | 0 | 0 |
| DOC501 | 2 | 1 | 1 | 0 | 0 |
20 tasks
Lee-W
force-pushed
the
autofix-AIR302
branch
3 times, most recently
from
17ddcf4 to
e4d614d
Compare
Lee-W
force-pushed
the
autofix-AIR302
branch
3 times, most recently
from
d12e9e3 to
773b73a
Compare
|
Gentle reminder @dhruvmanila |
dhruvmanila
reviewed
Mar 31, 2025
There was a problem hiding this comment.
We usually don't include any logic in mod.rs for a plugin. Any common logic to multiple rules in a plugin would go under airflow/helpers.rs file.
But, I see that the logic in this file is same as the one from numpy_2_0_deprecation.rs. I'd suggest to reuse the same infrastructure to avoid code duplication. What you can do is move the necessary code from numpy/rules/numpy_2_0_deprecation.rs to numpy/helpers.rs, make the necessary APIs pub(crate) and reuse them in airflow/rules/removal_in_3.rs.
There was a problem hiding this comment.
I'm trying to extract some of the common code to numpy/helpers.rs and airflow/helper.rs. But the structure is a bit different. Not sure whether we can reuse the whole is_guarded_by_try_except logic here
There was a problem hiding this comment.
Oh I see. It's fine to not reuse is_guarded_by_try_accept then and have a logic specific to Airflow.
dhruvmanila
reviewed
Mar 31, 2025
.../airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_class_attribute.py.snap
Outdated
Show resolved
Hide resolved
Lee-W
commented
Apr 1, 2025
| @@ -0,0 +1,64 @@ | |||
| use crate::rules::numpy::helpers::ImportSearcher; | |||
There was a problem hiding this comment.
This is moved here so that we can reused it after we reorg airflow rules
Lee-W
commented
Apr 1, 2025
| Airflow3Removal { | ||
| deprecated: qualified_name.to_string(), | ||
| replacement, | ||
| replacement: replacement.clone(), |
There was a problem hiding this comment.
I tried to move the fix a bit but did not succeeded. Should we refactor the fix here as well?
There was a problem hiding this comment.
I'm not sure I follow here. Do you mean to avoid the clone here that I suggested in your other PRs?
There was a problem hiding this comment.
If it requires too much effort then it's fine to avoid making the change.
There was a problem hiding this comment.
Do you mean to avoid the clone here that I suggested in your other PRs?
Yep
If it requires too much effort then it's fine to avoid making the change.
We can probably do that in a following PR
Lee-W
force-pushed
the
autofix-AIR302
branch
3 times, most recently
from
3493e97 to
28aa74d
Compare
Lee-W
added a commit
to astronomer/airflow
that referenced
this pull request
Apr 1, 2025
Lee-W
added a commit
to astronomer/airflow
that referenced
this pull request
Apr 1, 2025
Lee-W
added a commit
to astronomer/airflow
that referenced
this pull request
Apr 1, 2025
Lee-W
added a commit
to astronomer/airflow
that referenced
this pull request
Apr 2, 2025
dhruvmanila
reviewed
Apr 2, 2025
| semantic: &SemanticModel, | ||
| ) -> bool { | ||
| match expr { | ||
| Expr::Name(ExprName { id, .. }) => { |
There was a problem hiding this comment.
Is this always going to be a Name node? Can it be an Attribute node?
For example, in NumPy deprecation rules, there's this test case:
ruff/crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py
Lines 71 to 74 in ae2cf91
Is something like that possible? It's totally fine if you don't want to support something like that.
There was a problem hiding this comment.
We'll want to add it. But I'm thinking of addressing it in another PR. @sunank200 will take over during the time I'm out.
There was a problem hiding this comment.
Sounds good. Should I go merge this PR then?
Enjoy your time off!
There was a problem hiding this comment.
Yep, it would be super helpful if we could merge it first (also unblock a few draft PR). Let me resolve the conflict now!
I'm thinking of making it another PR. @sunank200 will take over during the time I'm out. |
dhruvmanila
changed the title
airflow] Add autofix infrastructure to AIR302 name checks
…` and skip try block logic
…it might be used to AIR303
dcreager
added a commit
that referenced
this pull request
Apr 3, 2025
* origin/main: (35 commits) [red-knot] Callable types are disjoint from literals (#17160) [red-knot] Fix inference for `pow` between two literal integers (#17161) [red-knot] Add GitHub PR annotations when mdtests fail in CI (#17150) [red-knot] Fix equivalence of differently ordered unions that contain `Callable` types (#17145) [red-knot] Add initial set of tests for unreachable code (#17159) [`airflow`] Move `AIR302` to `AIR301` and `AIR303` to `AIR302` (#17151) ruff_db: simplify lifetimes on `DiagnosticDisplay` [red-knot] Detect division-by-zero in unions and intersections (#17157) [`airflow`] Add autofix infrastructure to `AIR302` name checks (#16965) [`flake8-bandit`] Mark `str` and `list[str]` literals as trusted input (`S603`) (#17136) [`airflow`] Add autofix for `AIR302` attribute checks (#16977) [`airflow`] Extend `AIR302` with additional symbols (#17085) [`airflow`] Move `AIR301` to `AIR002` (#16978) [`airflow`] Add autofix for `AIR302` method checks (#16976) ruff_db: switch diagnostic rendering over to `std::fmt::Display` [red-knot] Add 'Goto type definition' to the playground (#17055) red_knot_ide: update snapshots red_knot_python_semantic: remove comment about `TypeCheckDiagnostic` ruff_db: delete most of the old diagnostic code red_knot: use `Diagnostic` inside of red knot ...
dcreager
added a commit
that referenced
this pull request
Apr 3, 2025
* origin/main: (82 commits) [red-knot] Fix more [redundant-cast] false positives (#17170) [red-knot] Three-argument type-calls take 'str' as the first argument (#17168) Control flow: `return` and `raise` (#17121) Bump 0.11.3 (#17173) [red-knot] Improve `Debug` implementation for `semantic_index::SymbolTable` (#17172) [red-knot] Fix `str(…)` calls (#17163) [red-knot] visibility_constraint analysis for match cases (#17077) [red-knot] Fix playground crashes when diagnostics are stale (#17165) [red-knot] Callable types are disjoint from literals (#17160) [red-knot] Fix inference for `pow` between two literal integers (#17161) [red-knot] Add GitHub PR annotations when mdtests fail in CI (#17150) [red-knot] Fix equivalence of differently ordered unions that contain `Callable` types (#17145) [red-knot] Add initial set of tests for unreachable code (#17159) [`airflow`] Move `AIR302` to `AIR301` and `AIR303` to `AIR302` (#17151) ruff_db: simplify lifetimes on `DiagnosticDisplay` [red-knot] Detect division-by-zero in unions and intersections (#17157) [`airflow`] Add autofix infrastructure to `AIR302` name checks (#16965) [`flake8-bandit`] Mark `str` and `list[str]` literals as trusted input (`S603`) (#17136) [`airflow`] Add autofix for `AIR302` attribute checks (#16977) [`airflow`] Extend `AIR302` with additional symbols (#17085) ...
maxmynter
pushed a commit
to maxmynter/ruff
that referenced
this pull request
Apr 3, 2025
…l-sh#16965) ## Summary Add autofix infrastructure to `AIR302` name checks and use this logic to fix`"airflow", "api_connexion", "security", "requires_access_dataset"`, `"airflow", "Dataset"` and `"airflow", "datasets", "Dataset"` ## Test Plan The existing test fixture reflects the update
Lee-W
added a commit
to astronomer/airflow
that referenced
this pull request
Apr 4, 2025
Lee-W
added a commit
to apache/airflow
that referenced
this pull request
Apr 7, 2025
simonprydden
pushed a commit
to simonprydden/airflow
that referenced
this pull request
Apr 8, 2025
kosteev
pushed a commit
to GoogleCloudPlatform/composer-airflow
that referenced
this pull request
May 29, 2025
…#16965 (#48618) GitOrigin-RevId: 59d765e0a9f5dcc6d4c0c5b93f7b7a55e67a1868
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Add autofix logic to
"airflow", "api_connexion", "security", "requires_access_dataset","airflow", "Dataset"and"airflow", "datasets", "Dataset"Test Plan
The existing test fixture reflects the update