-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
[airflow
] Fix ImportPathMoved
/ ProviderName
misuse (AIR303
)
#16013
Conversation
I'm thinking of splitting related test cases into separate files in the following PRs. Otherwise, it's a bit difficult to check the test cases. |
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
AIR303 | 5 | 5 | 0 | 0 | 0 |
AIR302 | 5 | 0 | 5 | 0 | 0 |
I think that'd be useful. We usually prefer self-contained PRs as it's much easier to review. As per the PR description, it seems like this might be doing multiple things (Fix |
I love this idea! I was worried about creating too many small PRs. Thanks for letting me know! |
df6c21e
to
be0b104
Compare
I just splited this into 3 pull requests (including this one). 🙌 I may need to rebase a bit after one is merged, but I should be fine. |
be0b104
to
09660be
Compare
@Lee-W Is this ready to be reviewed? Is it rebased after the split PRs have been merged? |
Hi, yes, this is already based on those PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, minor one small question.
crates/ruff_linter/src/rules/airflow/rules/moved_to_provider_in_3.rs
Outdated
Show resolved
Hide resolved
airflow
] Fix ImportPathMoved
/ ProviderName
misuse (AIR303
)
Summary
["airflow", "config_templates", "default_celery", "DEFAULT_CELERY_CONFIG"]
, should useProviderName
. In contrast, module paths like"airflow", "operators", "weekday", ...
should useImportPathMoved
. Misuse may lead to incorrect detection.Test Plan
update test fixture