Skip to content

[airflow] Get rid of Replacement::Name and replace them with Replacement::AutoImport for enabling auto fixing (AIR301, AIR311)#17941

Merged
ntBre merged 3 commits intoastral-sh:mainfrom
astronomer:refactor-AIR301-AIR311
May 14, 2025
Merged

[airflow] Get rid of Replacement::Name and replace them with Replacement::AutoImport for enabling auto fixing (AIR301, AIR311)#17941
ntBre merged 3 commits intoastral-sh:mainfrom
astronomer:refactor-AIR301-AIR311

Conversation

@Lee-W
Copy link
Contributor

@Lee-W Lee-W commented May 8, 2025

Summary

Similiar to #17941.

Replacement::Name was designed for linting only. Now, we also want to fix the user code. It would be easier to replace it with a better AutoImport struct whenever possible.

On the other hand, AIR301 and AIR311 contain attribute changes that can still use a struct like Replacement::Name. To reduce the confusion, I also updated it as Replacement::AttrName

Some of the original Replacement::Name has been replaced as Replacement::Message as they're not directly mapping and the message has now been moved to help

Test Plan

The test fixtures have been updated

@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+2 -2 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

apache/airflow (+2 -2 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ providers/fab/tests/unit/fab/auth_manager/api_fastapi/conftest.py:27:26: AIR301 `appbuilder` is removed in Airflow 3.0
- providers/fab/tests/unit/fab/auth_manager/api_fastapi/conftest.py:27:26: AIR301 `appbuilder` is removed in Airflow 3.0; The constructor takes no parameter now
+ providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py:99:26: AIR301 `appbuilder` is removed in Airflow 3.0
- providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py:99:26: AIR301 `appbuilder` is removed in Airflow 3.0; The constructor takes no parameter now

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
AIR301 4 2 2 0 0

@Lee-W Lee-W changed the title Refactor air301 air311 [airflow] Get rid of Replacement::Name and replace them with Replacement::AutoImport for enabling auto fixing (AIR302, AIR312) May 8, 2025
@Lee-W Lee-W marked this pull request as ready for review May 8, 2025 12:04
@Lee-W Lee-W mentioned this pull request May 8, 2025
2 tasks
@Lee-W Lee-W force-pushed the refactor-AIR301-AIR311 branch from 6c4f3dd to 4968eb0 Compare May 9, 2025 08:10
@Lee-W Lee-W changed the title [airflow] Get rid of Replacement::Name and replace them with Replacement::AutoImport for enabling auto fixing (AIR302, AIR312) [airflow] Get rid of Replacement::Name and replace them with Replacement::AutoImport for enabling auto fixing (AIR301, AIR311) May 9, 2025
@MichaReiser MichaReiser requested a review from ntBre May 12, 2025 07:42
@Lee-W Lee-W force-pushed the refactor-AIR301-AIR311 branch from 4968eb0 to b22e94c Compare May 12, 2025 10:55
@ntBre ntBre added rule Implementing or modifying a lint rule preview Related to preview mode features labels May 13, 2025
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

This looks good to me, just one question!

Comment on lines +73 to +74
Replacement::AttrName(name) => Some(format!("Use `{name}` instead")),
Replacement::Message(message) => Some((*message).to_string()),
Copy link
Contributor

@ntBre ntBre May 13, 2025

Choose a reason for hiding this comment

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

Do these need to be separate? It looks like all of the Replacement::Messages down below include Use ... instead in their messages, so I think you could at least combine these match arms if not the entire variants.

Suggested change
Replacement::AttrName(name) => Some(format!("Use `{name}` instead")),
Replacement::Message(message) => Some((*message).to_string()),
Replacement::AttrName(name) | Replacement::Message(name) => Some(format!("Use `{name}` instead")),

Or do you have plans to make the messages more different later on?

Copy link
Contributor Author

@Lee-W Lee-W May 14, 2025

Choose a reason for hiding this comment

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

There are one or two exceptions now. So it's still needed

["decorators", "apply_defaults"] => Replacement::Message(
"`apply_defaults` is now unconditionally done and can be safely removed.",
),

I'm thinking of merging the message into other types later on so that we can provide extra information for them as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thanks, I missed that one. I'll merge this!

@ntBre ntBre merged commit 2e94d37 into astral-sh:main May 14, 2025
34 checks passed
Glyphack pushed a commit to Glyphack/ruff that referenced this pull request May 21, 2025
…lacement::AutoImport` for enabling auto fixing (`AIR301`, `AIR311`) (astral-sh#17941)

<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

<!-- What's the purpose of the change? What does it do, and why? -->

Similiar to astral-sh#17941.

`Replacement::Name` was designed for linting only. Now, we also want to
fix the user code. It would be easier to replace it with a better
AutoImport struct whenever possible.

On the other hand, `AIR301` and `AIR311` contain attribute changes that
can still use a struct like `Replacement::Name`. To reduce the
confusion, I also updated it as `Replacement::AttrName`

Some of the original `Replacement::Name` has been replaced as
`Replacement::Message` as they're not directly mapping and the message
has now been moved to `help`


## Test Plan

<!-- How was it tested? -->

The test fixtures have been updated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants