Thank you! I'm happy to give this a review, but I like to check in with @Lee-W first :)

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Sure, will do it early next week. We're establishing the process how we contribute to ruff from airflow community. But Daniel's PR is most likely correct haha. Will take a deeper look 👀

Sure, will do it early next week. We're establishing the process how we contribute to ruff from airflow community. But Daniel's PR is most likely correct haha. Will take a deeper look 👀

All credit goes to claude

@Lee-W if we are also removing airflow.metrics and airflow.stats in 3.2, would it make sense to add these to the same ruff rule?

@Lee-W if we are also removing airflow.metrics and airflow.stats in 3.2, would it make sense to add these to the same ruff rule?

yep, we group logics with the same purpose into one rule

@dstandish do you think you'll have time in the near future to address the PR feedback? If not, that's completely fine, but I would then go ahead and close the PR. It makes it easier for us to track which work is currently in progress.

Below is a review of adherence to the new-airflow-rule SKILL:

Code Review: AIR331 — airflow.traces removal in Airflow 3.2

PR: #23914
Rule: AIR331 — flags imports from airflow.traces, removed in Airflow 3.2

Overview

The PR adds a new AIR331 lint rule that detects usage of symbols from airflow.traces.*, which was removed in Airflow 3.2. The implementation follows the general pattern of existing removal rules (AIR301, AIR321). The scope is narrow and well-defined: one entire module tree removed with no migration path.

Issues

1. Missing is_guarded_by_try_except guard (correctness)

Every other removal rule (AIR301, AIR321) guards against false positives on conditional imports:

try:
    from airflow.traces.tracer import Trace
except ImportError:
    Trace = None

removal_in_3_2.rs skips this check entirely. Add it:

use crate::rules::airflow::helpers::is_guarded_by_try_except;

// inside check_name(), before report_diagnostic:
if is_guarded_by_try_except(expr, module, name, semantic) {
    return;
}

Because the match uses ["airflow", "traces", ..] (a wildcard), you'll need to extract the specific module/name from qualified_name.segments() to pass the correct values to is_guarded_by_try_except.

2. preview_since should use "NEXT_RUFF_VERSION" placeholder (convention)

// current
#[violation_metadata(preview_since = "0.15.1")]

// should be
#[violation_metadata(preview_since = "NEXT_RUFF_VERSION")]

Although moved_in_3_1.rs also uses "0.15.1" (which appears to be an already-merged rule), new rules added in this PR should use "NEXT_RUFF_VERSION" so the version gets filled in at release time.

3. Dead code in fix_title (minor)

FIX_AVAILABILITY is None, so fix_title() is only called as a help: hint. But since only Replacement::None is ever stored in Airflow32Removal, the Replacement::Message arm in fix_title is unreachable dead code:

fn fix_title(&self) -> Option<String> {
    let Airflow32Removal { replacement, .. } = self;
    match replacement {
        Replacement::None => None,
        Replacement::Message(message) => Some((*message).to_string()), // never reached
        _ => None,
    }
}

Since no replacement exists for any airflow.traces symbol, fix_title can simply be removed (the Violation trait's default returns None). If future symbols with a Replacement::Message are added, re-introduce it then.

4. Match pattern is overly broad without enumerating known symbols (robustness)

["airflow", "traces", ..] matches any depth under airflow.traces, including hypothetical third-party code that happens to re-export under that namespace or future additions that may not actually be removed. Compare with AIR321, which enumerates each moved symbol explicitly.

Recommendation: enumerate the known removed symbols explicitly, matching only what was verified to be removed in 3.2:

["airflow", "traces", "otel_tracer"]
| ["airflow", "traces", "otel_tracer", "get_otel_tracer"]
| ["airflow", "traces", "NO_TRACE_ID"]
| ["airflow", "traces", "tracer", "Trace"]
| ["airflow", "traces", "tracer", "add_debug_span"]
| ["airflow", "traces", "utils", "gen_span_id_from_ti_key"] => Replacement::None,

If the intent is to flag the entire module (any access), document that in a comment.

5. Test fixture missing try-except guarded cases (test coverage)

The fixture has positive cases and non-airflow negative cases, but no cases guarded by try/except ImportError. Once is_guarded_by_try_except is added (issue #1), add fixture entries like:

# These should NOT trigger (guarded conditional import):
try:
    from airflow.traces.tracer import Trace
except ImportError:
    Trace = None

6. Test case ordering in mod.rs (nit)

// current (out of order):
#[test_case(Rule::Airflow31Moved, Path::new("AIR321_names.py"))]
#[test_case(Rule::Airflow32Removal, Path::new("AIR331_names.py"))]   // ← inserted here
#[test_case(Rule::Airflow3MovedToProvider, Path::new("AIR302_amazon.py"))]

AIR331 is numerically after AIR302–AIR312. Move the AIR331 test case after the AIR321 block, following numerical order.

7. Import ordering in removal_in_3_2.rs (style)

The file puts use crate::... before use ruff_* crates, which is the reverse of every other rule file. cargo fmt won't reorder across groups, so this must be fixed manually:

// conventional order used by all other airflow rules:
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::{Expr, ExprAttribute, ExprName};
use ruff_python_semantic::Modules;
use ruff_text_size::TextRange;

use crate::checkers::ast::Checker;
use crate::rules::airflow::helpers::Replacement;
use crate::{FixAvailability, Violation};

8. Documentation missing Use instead: section (docs convention)

The skill requires a Use instead: block in rule docs. Since there is no replacement, a note should be added:

/// ## Example
/// ```python
/// from airflow.traces.tracer import Trace
/// ```
///
/// Use instead:
/// Remove the import. The `airflow.traces` module has no replacement in Airflow 3.2.

Or simply state in "Why is this bad?" that there is no migration path, and omit the Use instead: block with an explanatory comment in the source.

Summary Table

Items 1 and 4 are the most important to address before merge. The rest are style/convention fixes.