From dcfb7533be69291c59960f4085761c978bd8ded3 Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Mon, 25 Nov 2024 14:45:15 +0800 Subject: [PATCH 1/5] Implement Airflow rule for 3.0 removals Airflow 3.0 removes various deprecated functions, members, modules, and other values. They have been deprecated in 2.x, but the removal causes incompatibilities that we want to detect. --- .../resources/test/fixtures/airflow/AIR302.py | 12 +++ .../src/checkers/ast/analyze/expression.rs | 6 ++ crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/airflow/mod.rs | 1 + .../src/rules/airflow/rules/mod.rs | 2 + .../src/rules/airflow/rules/removal_in_3.rs | 92 +++++++++++++++++++ ...les__airflow__tests__AIR302_AIR302.py.snap | 38 ++++++++ ruff.schema.json | 1 + 8 files changed, 153 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/airflow/AIR302.py create mode 100644 crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs create mode 100644 crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/airflow/AIR302.py b/crates/ruff_linter/resources/test/fixtures/airflow/AIR302.py new file mode 100644 index 0000000000000..299c87b9feef1 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/airflow/AIR302.py @@ -0,0 +1,12 @@ +from airflow.utils import dates +from airflow.utils.dates import date_range, datetime_to_nano, days_ago + +date_range +days_ago + +dates.date_range +dates.days_ago + +# This one was not deprecated. +datetime_to_nano +dates.datetime_to_nano diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 304e142ba9ff0..2c722c2368515 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -220,6 +220,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::RegexFlagAlias) { refurb::rules::regex_flag_alias(checker, expr); } + if checker.enabled(Rule::Airflow3Removal) { + airflow::rules::removed_in_3(checker, expr); + } // Ex) List[...] if checker.any_enabled(&[ @@ -380,6 +383,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::ByteStringUsage) { flake8_pyi::rules::bytestring_attribute(checker, expr); } + if checker.enabled(Rule::Airflow3Removal) { + airflow::rules::removed_in_3(checker, expr); + } } Expr::Call( call @ ast::ExprCall { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index dc28b55489446..dd9426193701c 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1040,6 +1040,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { // airflow (Airflow, "001") => (RuleGroup::Stable, rules::airflow::rules::AirflowVariableNameTaskIdMismatch), (Airflow, "301") => (RuleGroup::Preview, rules::airflow::rules::AirflowDagNoScheduleArgument), + (Airflow, "302") => (RuleGroup::Preview, rules::airflow::rules::Airflow3Removal), // perflint (Perflint, "101") => (RuleGroup::Stable, rules::perflint::rules::UnnecessaryListCast), diff --git a/crates/ruff_linter/src/rules/airflow/mod.rs b/crates/ruff_linter/src/rules/airflow/mod.rs index 4e4130766022f..2ab4d6cefb16e 100644 --- a/crates/ruff_linter/src/rules/airflow/mod.rs +++ b/crates/ruff_linter/src/rules/airflow/mod.rs @@ -14,6 +14,7 @@ mod tests { #[test_case(Rule::AirflowVariableNameTaskIdMismatch, Path::new("AIR001.py"))] #[test_case(Rule::AirflowDagNoScheduleArgument, Path::new("AIR301.py"))] + #[test_case(Rule::Airflow3Removal, Path::new("AIR302.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/airflow/rules/mod.rs b/crates/ruff_linter/src/rules/airflow/rules/mod.rs index b01391092ca77..e5d1c83fb6dd8 100644 --- a/crates/ruff_linter/src/rules/airflow/rules/mod.rs +++ b/crates/ruff_linter/src/rules/airflow/rules/mod.rs @@ -1,5 +1,7 @@ pub(crate) use dag_schedule_argument::*; +pub(crate) use removal_in_3::*; pub(crate) use task_variable_name::*; mod dag_schedule_argument; +mod removal_in_3; mod task_variable_name; diff --git a/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs b/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs new file mode 100644 index 0000000000000..be879075b48cd --- /dev/null +++ b/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs @@ -0,0 +1,92 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::Expr; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +#[derive(Debug, Eq, PartialEq)] +enum Replacement { + None, + Name(String), +} + +impl Replacement { + fn name(name: impl Into) -> Self { + Self::Name(name.into()) + } +} + +/// ## What it does +/// Checks for uses of deprecated Airflow functions and values. +/// +/// ## Why is this bad? +/// Airflow 3.0 removed various deprecated functions, members, and other +/// values. Some have more modern replacements. Others are considered too niche +/// and not worth to be maintained in Airflow. +/// +/// ## Example +/// ```python +/// from airflow.utils.dates import days_ago +/// +/// +/// yesterday = days_ago(today, 1) +/// ``` +/// +/// Use instead: +/// ```python +/// from datetime import timedelta +/// +/// +/// yesterday = today - timedelta(days=1) +/// ``` +#[violation] +pub struct Airflow3Removal { + deprecated: String, + replacement: Replacement, +} + +impl Violation for Airflow3Removal { + #[derive_message_formats] + fn message(&self) -> String { + let Airflow3Removal { + deprecated, + replacement, + } = self; + match replacement { + Replacement::None => format!("`{deprecated}` is removed in Airflow 3.0"), + Replacement::Name(name) => { + format!("`{deprecated}` is removed in Airflow 3.0; use {name} instead") + } + } + } +} + +/// AIR302 +pub(crate) fn removed_in_3(checker: &mut Checker, expr: &Expr) { + let Some((deprecated, replacement)) = + checker + .semantic() + .resolve_qualified_name(expr) + .and_then(|qualname| match qualname.segments() { + ["airflow", "utils", "dates", "date_range"] => { + Some((qualname.to_string(), Replacement::None)) + } + ["airflow", "utils", "dates", "days_ago"] => Some(( + qualname.to_string(), + Replacement::name("datetime.timedelta()"), + )), + _ => None, + }) + else { + return; + }; + + checker.diagnostics.push(Diagnostic::new( + Airflow3Removal { + deprecated, + replacement, + }, + expr.range(), + )); +} diff --git a/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302.py.snap b/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302.py.snap new file mode 100644 index 0000000000000..c9281e03d9136 --- /dev/null +++ b/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302.py.snap @@ -0,0 +1,38 @@ +--- +source: crates/ruff_linter/src/rules/airflow/mod.rs +--- +AIR302.py:4:1: AIR302 `airflow.utils.dates.date_range` is removed in Airflow 3.0 + | +2 | from airflow.utils.dates import date_range, datetime_to_nano, days_ago +3 | +4 | date_range + | ^^^^^^^^^^ AIR302 +5 | days_ago + | + +AIR302.py:5:1: AIR302 `airflow.utils.dates.days_ago` is removed in Airflow 3.0; use datetime.timedelta() instead + | +4 | date_range +5 | days_ago + | ^^^^^^^^ AIR302 +6 | +7 | dates.date_range + | + +AIR302.py:7:1: AIR302 `airflow.utils.dates.date_range` is removed in Airflow 3.0 + | +5 | days_ago +6 | +7 | dates.date_range + | ^^^^^^^^^^^^^^^^ AIR302 +8 | dates.days_ago + | + +AIR302.py:8:1: AIR302 `airflow.utils.dates.days_ago` is removed in Airflow 3.0; use datetime.timedelta() instead + | + 7 | dates.date_range + 8 | dates.days_ago + | ^^^^^^^^^^^^^^ AIR302 + 9 | +10 | # This one was not deprecated. + | diff --git a/ruff.schema.json b/ruff.schema.json index 40cdf7e96d4a9..025f8e3011999 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2797,6 +2797,7 @@ "AIR3", "AIR30", "AIR301", + "AIR302", "ALL", "ANN", "ANN0", From a25fbf9686350bb0ec581979705a70d7d146f8ac Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Wed, 27 Nov 2024 18:09:40 +0800 Subject: [PATCH 2/5] Support both name removals and arg removals --- .../test/fixtures/airflow/AIR302_args.py | 23 +++++ .../airflow/{AIR302.py => AIR302_names.py} | 0 .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/rules/airflow/mod.rs | 3 +- .../src/rules/airflow/rules/removal_in_3.rs | 87 ++++++++++++++----- ...les__airflow__tests__AIR302_AIR302.py.snap | 38 -------- ...airflow__tests__AIR302_AIR302_args.py.snap | 36 ++++++++ ...irflow__tests__AIR302_AIR302_names.py.snap | 38 ++++++++ 8 files changed, 168 insertions(+), 60 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/airflow/AIR302_args.py rename crates/ruff_linter/resources/test/fixtures/airflow/{AIR302.py => AIR302_names.py} (100%) delete mode 100644 crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302.py.snap create mode 100644 crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_args.py.snap create mode 100644 crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_names.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/airflow/AIR302_args.py b/crates/ruff_linter/resources/test/fixtures/airflow/AIR302_args.py new file mode 100644 index 0000000000000..2296784762eaa --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/airflow/AIR302_args.py @@ -0,0 +1,23 @@ +from airflow import DAG, dag +from airflow.timetables.simple import NullTimetable + +DAG(dag_id="class_schedule", schedule="@hourly") + +DAG(dag_id="class_schedule_interval", schedule_interval="@hourly") + +DAG(dag_id="class_timetable", timetable=NullTimetable()) + + +@dag(schedule="0 * * * *") +def decorator_schedule(): + pass + + +@dag(schedule_interval="0 * * * *") +def decorator_schedule_interval(): + pass + + +@dag(timetable=NullTimetable()) +def decorator_timetable(): + pass diff --git a/crates/ruff_linter/resources/test/fixtures/airflow/AIR302.py b/crates/ruff_linter/resources/test/fixtures/airflow/AIR302_names.py similarity index 100% rename from crates/ruff_linter/resources/test/fixtures/airflow/AIR302.py rename to crates/ruff_linter/resources/test/fixtures/airflow/AIR302_names.py diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 2c722c2368515..ae3dfdc31b859 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1090,6 +1090,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryRegularExpression) { ruff::rules::unnecessary_regular_expression(checker, call); } + if checker.enabled(Rule::Airflow3Removal) { + airflow::rules::removed_in_3(checker, expr); + } } Expr::Dict(dict) => { if checker.any_enabled(&[ diff --git a/crates/ruff_linter/src/rules/airflow/mod.rs b/crates/ruff_linter/src/rules/airflow/mod.rs index 2ab4d6cefb16e..1869b0888b445 100644 --- a/crates/ruff_linter/src/rules/airflow/mod.rs +++ b/crates/ruff_linter/src/rules/airflow/mod.rs @@ -14,7 +14,8 @@ mod tests { #[test_case(Rule::AirflowVariableNameTaskIdMismatch, Path::new("AIR001.py"))] #[test_case(Rule::AirflowDagNoScheduleArgument, Path::new("AIR301.py"))] - #[test_case(Rule::Airflow3Removal, Path::new("AIR302.py"))] + #[test_case(Rule::Airflow3Removal, Path::new("AIR302_args.py"))] + #[test_case(Rule::Airflow3Removal, Path::new("AIR302_names.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs b/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs index be879075b48cd..a4aa9f73a20a3 100644 --- a/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs +++ b/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs @@ -1,6 +1,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::Expr; +use ruff_python_ast::{name::QualifiedName, Arguments, Expr, ExprAttribute, ExprCall}; +use ruff_python_semantic::Modules; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -11,12 +12,6 @@ enum Replacement { Name(String), } -impl Replacement { - fn name(name: impl Into) -> Self { - Self::Name(name.into()) - } -} - /// ## What it does /// Checks for uses of deprecated Airflow functions and values. /// @@ -62,9 +57,41 @@ impl Violation for Airflow3Removal { } } -/// AIR302 -pub(crate) fn removed_in_3(checker: &mut Checker, expr: &Expr) { - let Some((deprecated, replacement)) = +struct DeprecatedArgument(Vec<&'static str>, Option<&'static str>); + +fn removed_argument(checker: &mut Checker, qualname: &QualifiedName, arguments: &Arguments) { + let deprecations = match qualname.segments() { + ["airflow", .., "DAG" | "dag"] => vec![DeprecatedArgument( + vec!["timetable", "schedule_interval"], + Some("schedule"), + )], + _ => { + return; + } + }; + for deprecation in deprecations { + for arg in deprecation.0 { + if let Some(keyword) = arguments.find_keyword(arg) { + checker.diagnostics.push(Diagnostic::new( + Airflow3Removal { + deprecated: arg.to_string(), + replacement: match deprecation.1 { + Some(name) => Replacement::Name(name.to_owned()), + None => Replacement::None, + }, + }, + keyword + .arg + .as_ref() + .map_or_else(|| keyword.range(), Ranged::range), + )); + }; + } + } +} + +fn removed_name(checker: &mut Checker, expr: &Expr, ranged: impl Ranged) { + let result = checker .semantic() .resolve_qualified_name(expr) @@ -74,19 +101,37 @@ pub(crate) fn removed_in_3(checker: &mut Checker, expr: &Expr) { } ["airflow", "utils", "dates", "days_ago"] => Some(( qualname.to_string(), - Replacement::name("datetime.timedelta()"), + Replacement::Name("datetime.timedelta()".to_string()), )), _ => None, - }) - else { + }); + if let Some((deprecated, replacement)) = result { + checker.diagnostics.push(Diagnostic::new( + Airflow3Removal { + deprecated, + replacement, + }, + ranged.range(), + )); + } +} + +/// AIR302 +pub(crate) fn removed_in_3(checker: &mut Checker, expr: &Expr) { + if !checker.semantic().seen_module(Modules::AIRFLOW) { return; - }; + } - checker.diagnostics.push(Diagnostic::new( - Airflow3Removal { - deprecated, - replacement, - }, - expr.range(), - )); + match expr { + Expr::Call(ExprCall { + func, arguments, .. + }) => { + if let Some(qualname) = checker.semantic().resolve_qualified_name(func) { + removed_argument(checker, &qualname, arguments); + }; + } + Expr::Attribute(ExprAttribute { attr: ranged, .. }) => removed_name(checker, expr, ranged), + ranged @ Expr::Name(_) => removed_name(checker, expr, ranged), + _ => {} + } } diff --git a/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302.py.snap b/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302.py.snap deleted file mode 100644 index c9281e03d9136..0000000000000 --- a/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302.py.snap +++ /dev/null @@ -1,38 +0,0 @@ ---- -source: crates/ruff_linter/src/rules/airflow/mod.rs ---- -AIR302.py:4:1: AIR302 `airflow.utils.dates.date_range` is removed in Airflow 3.0 - | -2 | from airflow.utils.dates import date_range, datetime_to_nano, days_ago -3 | -4 | date_range - | ^^^^^^^^^^ AIR302 -5 | days_ago - | - -AIR302.py:5:1: AIR302 `airflow.utils.dates.days_ago` is removed in Airflow 3.0; use datetime.timedelta() instead - | -4 | date_range -5 | days_ago - | ^^^^^^^^ AIR302 -6 | -7 | dates.date_range - | - -AIR302.py:7:1: AIR302 `airflow.utils.dates.date_range` is removed in Airflow 3.0 - | -5 | days_ago -6 | -7 | dates.date_range - | ^^^^^^^^^^^^^^^^ AIR302 -8 | dates.days_ago - | - -AIR302.py:8:1: AIR302 `airflow.utils.dates.days_ago` is removed in Airflow 3.0; use datetime.timedelta() instead - | - 7 | dates.date_range - 8 | dates.days_ago - | ^^^^^^^^^^^^^^ AIR302 - 9 | -10 | # This one was not deprecated. - | diff --git a/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_args.py.snap b/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_args.py.snap new file mode 100644 index 0000000000000..23725fc463073 --- /dev/null +++ b/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_args.py.snap @@ -0,0 +1,36 @@ +--- +source: crates/ruff_linter/src/rules/airflow/mod.rs +--- +AIR302_args.py:6:39: AIR302 `schedule_interval` is removed in Airflow 3.0; use schedule instead + | +4 | DAG(dag_id="class_schedule", schedule="@hourly") +5 | +6 | DAG(dag_id="class_schedule_interval", schedule_interval="@hourly") + | ^^^^^^^^^^^^^^^^^ AIR302 +7 | +8 | DAG(dag_id="class_timetable", timetable=NullTimetable()) + | + +AIR302_args.py:8:31: AIR302 `timetable` is removed in Airflow 3.0; use schedule instead + | +6 | DAG(dag_id="class_schedule_interval", schedule_interval="@hourly") +7 | +8 | DAG(dag_id="class_timetable", timetable=NullTimetable()) + | ^^^^^^^^^ AIR302 + | + +AIR302_args.py:16:6: AIR302 `schedule_interval` is removed in Airflow 3.0; use schedule instead + | +16 | @dag(schedule_interval="0 * * * *") + | ^^^^^^^^^^^^^^^^^ AIR302 +17 | def decorator_schedule_interval(): +18 | pass + | + +AIR302_args.py:21:6: AIR302 `timetable` is removed in Airflow 3.0; use schedule instead + | +21 | @dag(timetable=NullTimetable()) + | ^^^^^^^^^ AIR302 +22 | def decorator_timetable(): +23 | pass + | diff --git a/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_names.py.snap b/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_names.py.snap new file mode 100644 index 0000000000000..f97e1dc4f110f --- /dev/null +++ b/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_names.py.snap @@ -0,0 +1,38 @@ +--- +source: crates/ruff_linter/src/rules/airflow/mod.rs +--- +AIR302_names.py:4:1: AIR302 `airflow.utils.dates.date_range` is removed in Airflow 3.0 + | +2 | from airflow.utils.dates import date_range, datetime_to_nano, days_ago +3 | +4 | date_range + | ^^^^^^^^^^ AIR302 +5 | days_ago + | + +AIR302_names.py:5:1: AIR302 `airflow.utils.dates.days_ago` is removed in Airflow 3.0; use datetime.timedelta() instead + | +4 | date_range +5 | days_ago + | ^^^^^^^^ AIR302 +6 | +7 | dates.date_range + | + +AIR302_names.py:7:7: AIR302 `airflow.utils.dates.date_range` is removed in Airflow 3.0 + | +5 | days_ago +6 | +7 | dates.date_range + | ^^^^^^^^^^ AIR302 +8 | dates.days_ago + | + +AIR302_names.py:8:7: AIR302 `airflow.utils.dates.days_ago` is removed in Airflow 3.0; use datetime.timedelta() instead + | + 7 | dates.date_range + 8 | dates.days_ago + | ^^^^^^^^ AIR302 + 9 | +10 | # This one was not deprecated. + | From 952414d9e57701d99dc8f31462a25c072e627e08 Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Wed, 27 Nov 2024 18:13:30 +0800 Subject: [PATCH 3/5] Use ViolationMetadata --- crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs b/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs index a4aa9f73a20a3..6ed8d898beace 100644 --- a/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs +++ b/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs @@ -1,5 +1,5 @@ use ruff_diagnostics::{Diagnostic, Violation}; -use ruff_macros::{derive_message_formats, violation}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::{name::QualifiedName, Arguments, Expr, ExprAttribute, ExprCall}; use ruff_python_semantic::Modules; use ruff_text_size::Ranged; @@ -35,8 +35,8 @@ enum Replacement { /// /// yesterday = today - timedelta(days=1) /// ``` -#[violation] -pub struct Airflow3Removal { +#[derive(ViolationMetadata)] +pub(crate) struct Airflow3Removal { deprecated: String, replacement: Replacement, } From f3f561293f7288cd51375458075ae1fc434fb70a Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Thu, 28 Nov 2024 21:26:39 +0800 Subject: [PATCH 4/5] Do not emit AIR301 with deprecated schedule arg When a deprecated schedule argument is found on a DAG, it is more appropriate to emit an AIR302 to signify a rename, instead of telling the user to add a 'schedule' argument (which won't work unless they also remove the deprecated argument). --- .../resources/test/fixtures/airflow/AIR301.py | 15 ++++++++++++++ .../airflow/rules/dag_schedule_argument.rs | 10 ++++++++-- ...les__airflow__tests__AIR301_AIR301.py.snap | 20 +++++++++---------- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/airflow/AIR301.py b/crates/ruff_linter/resources/test/fixtures/airflow/AIR301.py index 1841d3c192872..5ddbbb3e38bba 100644 --- a/crates/ruff_linter/resources/test/fixtures/airflow/AIR301.py +++ b/crates/ruff_linter/resources/test/fixtures/airflow/AIR301.py @@ -1,9 +1,14 @@ from airflow import DAG, dag +from airflow.timetables.simple import NullTimetable DAG(dag_id="class_default_schedule") DAG(dag_id="class_schedule", schedule="@hourly") +DAG(dag_id="class_schedule_interval", schedule_interval="@hourly") + +DAG(dag_id="class_timetable", timetable=NullTimetable()) + @dag() def decorator_default_schedule(): @@ -13,3 +18,13 @@ def decorator_default_schedule(): @dag(schedule="0 * * * *") def decorator_schedule(): pass + + +@dag(schedule_interval="0 * * * *") +def decorator_schedule_interval(): + pass + + +@dag(timetable=NullTimetable()) +def decorator_timetable(): + pass diff --git a/crates/ruff_linter/src/rules/airflow/rules/dag_schedule_argument.rs b/crates/ruff_linter/src/rules/airflow/rules/dag_schedule_argument.rs index b3ae01e527744..d8a9ec437c5f8 100644 --- a/crates/ruff_linter/src/rules/airflow/rules/dag_schedule_argument.rs +++ b/crates/ruff_linter/src/rules/airflow/rules/dag_schedule_argument.rs @@ -73,8 +73,14 @@ pub(crate) fn dag_no_schedule_argument(checker: &mut Checker, expr: &Expr) { return; } - // If there's a `schedule` keyword argument, we are good. - if arguments.find_keyword("schedule").is_some() { + // If there's a schedule keyword argument, we are good. + // This includes the canonical 'schedule', and the deprecated 'timetable' + // and 'schedule_interval'. Usages of deprecated schedule arguments are + // covered by AIR302. + if ["schedule", "schedule_interval", "timetable"] + .iter() + .any(|a| arguments.find_keyword(a).is_some()) + { return; } diff --git a/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR301_AIR301.py.snap b/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR301_AIR301.py.snap index b2afb063a40c2..d25605ac55561 100644 --- a/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR301_AIR301.py.snap +++ b/crates/ruff_linter/src/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR301_AIR301.py.snap @@ -1,20 +1,20 @@ --- source: crates/ruff_linter/src/rules/airflow/mod.rs --- -AIR301.py:3:1: AIR301 DAG should have an explicit `schedule` argument +AIR301.py:4:1: AIR301 DAG should have an explicit `schedule` argument | -1 | from airflow import DAG, dag -2 | -3 | DAG(dag_id="class_default_schedule") +2 | from airflow.timetables.simple import NullTimetable +3 | +4 | DAG(dag_id="class_default_schedule") | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AIR301 -4 | -5 | DAG(dag_id="class_schedule", schedule="@hourly") +5 | +6 | DAG(dag_id="class_schedule", schedule="@hourly") | -AIR301.py:8:2: AIR301 DAG should have an explicit `schedule` argument +AIR301.py:13:2: AIR301 DAG should have an explicit `schedule` argument | - 8 | @dag() +13 | @dag() | ^^^^^ AIR301 - 9 | def decorator_default_schedule(): -10 | pass +14 | def decorator_default_schedule(): +15 | pass | From 3f381e6d47bbd401a84665a8fddad31daf6b258c Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Mon, 2 Dec 2024 14:28:12 +0800 Subject: [PATCH 5/5] Avoid intermediate vectors --- .../src/rules/airflow/rules/removal_in_3.rs | 61 +++++++++++-------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs b/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs index 6ed8d898beace..527b5cd402275 100644 --- a/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs +++ b/crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs @@ -57,37 +57,44 @@ impl Violation for Airflow3Removal { } } -struct DeprecatedArgument(Vec<&'static str>, Option<&'static str>); +fn diagnostic_for_argument( + arguments: &Arguments, + deprecated: &str, + replacement: Option<&str>, +) -> Option { + let keyword = arguments.find_keyword(deprecated)?; + Some(Diagnostic::new( + Airflow3Removal { + deprecated: (*deprecated).to_string(), + replacement: match replacement { + Some(name) => Replacement::Name(name.to_owned()), + None => Replacement::None, + }, + }, + keyword + .arg + .as_ref() + .map_or_else(|| keyword.range(), Ranged::range), + )) +} fn removed_argument(checker: &mut Checker, qualname: &QualifiedName, arguments: &Arguments) { - let deprecations = match qualname.segments() { - ["airflow", .., "DAG" | "dag"] => vec![DeprecatedArgument( - vec!["timetable", "schedule_interval"], - Some("schedule"), - )], - _ => { - return; + #[allow(clippy::single_match)] + match qualname.segments() { + ["airflow", .., "DAG" | "dag"] => { + checker.diagnostics.extend(diagnostic_for_argument( + arguments, + "schedule_interval", + Some("schedule"), + )); + checker.diagnostics.extend(diagnostic_for_argument( + arguments, + "timetable", + Some("schedule"), + )); } + _ => {} }; - for deprecation in deprecations { - for arg in deprecation.0 { - if let Some(keyword) = arguments.find_keyword(arg) { - checker.diagnostics.push(Diagnostic::new( - Airflow3Removal { - deprecated: arg.to_string(), - replacement: match deprecation.1 { - Some(name) => Replacement::Name(name.to_owned()), - None => Replacement::None, - }, - }, - keyword - .arg - .as_ref() - .map_or_else(|| keyword.range(), Ranged::range), - )); - }; - } - } } fn removed_name(checker: &mut Checker, expr: &Expr, ranged: impl Ranged) {