Add AIR001: task variable name should be same as task_id arg #4687

jlaneve · 2023-05-27T20:03:38Z

Summary

This PR adds the first Ruff Airflow rule: AIR001. Related to #4421.

AIR001: An Airflow task's Python variable name should be the same as the task_id arg you give it. This follows https://github.com/BasPH/pylint-airflow C8300.

This PR also adds the groundwork for adding new Airflow-related rules!

Test Plan

Unit tests included in the PR.

crates/ruff/src/rules/airflow/mod.rs

jlaneve · 2023-05-27T20:07:36Z

crates/ruff/src/rules/airflow/rules/tasks.rs

+    // if the function doesn't come from airflow, we can't do anything
+    let func_name = match call.func.as_name_expr() {
+        Some(name) => name.id.as_str(),
+        None => return None,
+    };
+    let fully_qualified_func_name = match checker.ctx.find_binding(func_name) {
+        Some(call_path) => match call_path.kind.as_from_importation() {
+            Some(from_importation) => &from_importation.full_name,
+            None => return None,
+        },
+        None => return None,
+    };
+
+    if !fully_qualified_func_name.starts_with("airflow.") {
+        return None;
+    }


in theory this should only be run if the class being called inherits from airflow.models.baseoperator.BaseOperator, but I wanted to get something up to at least start a discussion. Also worth noting that I tried checker.ctx.resolve_call_path but couldn't get it to work... could certainly be user error though.

github-actions · 2023-05-27T20:37:29Z

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.06     14.9±0.04ms     2.7 MB/sec    1.00     14.1±0.05ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.03      3.5±0.00ms     4.7 MB/sec    1.00      3.4±0.00ms     4.9 MB/sec
linter/all-rules/numpy/globals.py          1.01    430.8±0.39µs     6.8 MB/sec    1.00    425.4±0.77µs     6.9 MB/sec
linter/all-rules/pydantic/types.py         1.05      6.1±0.01ms     4.2 MB/sec    1.00      5.8±0.01ms     4.4 MB/sec
linter/default-rules/large/dataset.py      1.09      7.5±0.02ms     5.4 MB/sec    1.00      6.9±0.01ms     5.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.05   1603.0±1.95µs    10.4 MB/sec    1.00   1522.9±3.15µs    10.9 MB/sec
linter/default-rules/numpy/globals.py      1.02    176.3±0.20µs    16.7 MB/sec    1.00    172.2±0.22µs    17.1 MB/sec
linter/default-rules/pydantic/types.py     1.07      3.3±0.01ms     7.6 MB/sec    1.00      3.1±0.01ms     8.2 MB/sec
parser/large/dataset.py                    1.00      5.2±0.00ms     7.9 MB/sec    1.00      5.1±0.00ms     7.9 MB/sec
parser/numpy/ctypeslib.py                  1.00   1021.1±2.41µs    16.3 MB/sec    1.00   1022.0±0.40µs    16.3 MB/sec
parser/numpy/globals.py                    1.00    105.9±0.20µs    27.9 MB/sec    1.00    105.5±0.34µs    28.0 MB/sec
parser/pydantic/types.py                   1.00      2.2±0.00ms    11.5 MB/sec    1.00      2.2±0.00ms    11.4 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     16.6±0.19ms     2.5 MB/sec    1.00     16.6±0.16ms     2.4 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.2±0.05ms     4.0 MB/sec    1.00      4.2±0.05ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    498.7±5.34µs     5.9 MB/sec    1.01   503.2±13.33µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.0±0.07ms     3.6 MB/sec    1.01      7.0±0.09ms     3.6 MB/sec
linter/default-rules/large/dataset.py      1.00      8.1±0.07ms     5.0 MB/sec    1.01      8.2±0.04ms     5.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1752.8±19.64µs     9.5 MB/sec    1.01  1771.5±18.23µs     9.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    202.1±2.77µs    14.6 MB/sec    1.03    207.5±4.94µs    14.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.7±0.04ms     6.9 MB/sec    1.01      3.7±0.05ms     6.8 MB/sec
parser/large/dataset.py                    1.15      7.4±0.04ms     5.5 MB/sec    1.00      6.5±0.04ms     6.3 MB/sec
parser/numpy/ctypeslib.py                  1.12  1374.7±15.24µs    12.1 MB/sec    1.00  1231.7±11.18µs    13.5 MB/sec
parser/numpy/globals.py                    1.08    136.1±1.68µs    21.7 MB/sec    1.00    125.8±2.02µs    23.5 MB/sec
parser/pydantic/types.py                   1.12      3.1±0.02ms     8.2 MB/sec    1.00      2.8±0.02ms     9.2 MB/sec

jlaneve · 2023-05-27T21:07:04Z

@charliermarsh any chance you can help with the CI issues? Not too sure what’s going on, this works fine locally and the code looks consistent with what I see elsewhere.

Feel free to push changes directly to this branch if it’s easier than adding comments inline!

charliermarsh · 2023-05-27T21:19:34Z

Not sure what that failure was -- we replaced .ctx with .semantic_model(), but your branch was also passing for me locally (since that change was past your branch's upstream). Maybe some kind of weird Cargo caching failure on CI? Regardless, I merged in main + changed the reference, so hopefully it passes now.

charliermarsh

Looks great @jlaneve! I went ahead and pushed some changes, but commenting on them here for clarity + transparency -- let me know if any of them are incorrect, confusing, etc. Thanks for getting this started!

charliermarsh · 2023-05-29T03:18:54Z

crates/ruff/src/checkers/ast/mod.rs

@@ -1670,13 +1666,22 @@ where
                if self.enabled(Rule::UnpackedListComprehension) {
                    pyupgrade::rules::unpacked_list_comprehension(self, targets, value);
                }
-


(Unrelated, but the newlines here felt inconsistent, so just removed.)

charliermarsh · 2023-05-29T03:19:26Z

crates/ruff/src/rules/airflow/rules/task_variable_name.rs

+    task_id: String,
+}
+
+impl Violation for AirflowVariableNameTaskIdMismatch {


I renamed this to prefix with "Airflow" to match the "Django" and "NumPy" conventions.

charliermarsh · 2023-05-29T03:19:34Z

crates/ruff/src/rules/airflow/rules/task_variable_name.rs

+    #[derive_message_formats]
+    fn message(&self) -> String {
+        let AirflowVariableNameTaskIdMismatch { task_id } = self;
+        format!("Task variable name should match the `task_id`: \"{task_id}\"")


(Added the task_id itself to the message.)

charliermarsh · 2023-05-29T03:21:10Z

crates/ruff/src/rules/airflow/rules/task_variable_name.rs

+    if !checker
+        .semantic_model()
+        .resolve_call_path(func)
+        .map_or(false, |call_path| matches!(call_path[0], "airflow"))


Modified this to use resolve_call_path, which will resolve to the fully qualified path -- so it'll do the right thing in cases like:

import airflow foo = airflow.PythonOperator(...) import airflow as Foo foo = Foo.PythonOperator(...)

charliermarsh · 2023-05-29T03:21:21Z

crates/ruff/src/rules/airflow/rules/task_variable_name.rs

+    // If the call doesn't have a `task_id` keyword argument, we can't do anything.
+    let keyword = keywords
+        .iter()
+        .find(|keyword| keyword.arg.as_ref().map_or(false, |arg| arg == "task_id"))?;


Can use ? to tighten this up a little.

charliermarsh · 2023-05-29T03:22:39Z

README.md

@@ -291,6 +291,7 @@ quality tools, including:
 - [pep8-naming](https://pypi.org/project/pep8-naming/)
 - [pydocstyle](https://pypi.org/project/pydocstyle/)
 - [pygrep-hooks](https://github.com/pre-commit/pygrep-hooks)
+- [pylint-airflow](https://pypi.org/project/pylint-airflow/)


I decided to link to the Pylint plugin, since this list refers to tools we've reimplemented, and it felt more appropriate than "Airflow" -- wdyt?

yeah this makes sense to me! hopefully we get to a point where we're linting for more than just pylint-airflow though 🙂

Add AIR001: task variable name should be same as task_id arg

ff812fe

jlaneve commented May 27, 2023

View reviewed changes

crates/ruff/src/rules/airflow/mod.rs Outdated Show resolved Hide resolved

jlaneve commented May 27, 2023

View reviewed changes

jlaneve added 2 commits May 27, 2023 16:48

fix clippy issues

541d798

rename file and generate code

560ff88

charliermarsh added 2 commits May 27, 2023 17:17

Merge branch 'main' into air001-task-name-aligned

f8481f4

Use .semantic_model()

fa93c70

jlaneve added 2 commits May 27, 2023 18:27

add airflow to readme

67adf70

update tests to follow standard

8708257

jlaneve marked this pull request as ready for review May 27, 2023 22:29

jlaneve and others added 4 commits May 27, 2023 18:36

update docs to be valid example

7776aca

fix docs formatting issue

f62a2f2

Merge branch 'main' into air001-task-name-aligned

403fb69

Minor tweaks to docs, implementation, naming

aed21fb

charliermarsh added the rule Implementing or modifying a lint rule label May 29, 2023

charliermarsh enabled auto-merge (squash) May 29, 2023 03:17

charliermarsh reviewed May 29, 2023

View reviewed changes

charliermarsh merged commit 68db74b into astral-sh:main May 29, 2023

chenrui333 mentioned this pull request Jun 6, 2023

ruff 0.0.272 Homebrew/homebrew-core#133013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AIR001: task variable name should be same as task_id arg #4687

Add AIR001: task variable name should be same as task_id arg #4687

jlaneve commented May 27, 2023 •

edited

Loading

jlaneve May 27, 2023

github-actions bot commented May 27, 2023 •

edited

Loading

jlaneve commented May 27, 2023 •

edited

Loading

charliermarsh commented May 27, 2023

charliermarsh left a comment

charliermarsh May 29, 2023

charliermarsh May 29, 2023

charliermarsh May 29, 2023

charliermarsh May 29, 2023

charliermarsh May 29, 2023

charliermarsh May 29, 2023

jlaneve May 29, 2023

Add AIR001: task variable name should be same as task_id arg #4687

Add AIR001: task variable name should be same as task_id arg #4687

Conversation

jlaneve commented May 27, 2023 • edited Loading

Summary

Test Plan

Choose a reason for hiding this comment

github-actions bot commented May 27, 2023 • edited Loading

PR Check Results

Benchmark

Linux

Windows

jlaneve commented May 27, 2023 • edited Loading

charliermarsh commented May 27, 2023

charliermarsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlaneve commented May 27, 2023 •

edited

Loading

github-actions bot commented May 27, 2023 •

edited

Loading

jlaneve commented May 27, 2023 •

edited

Loading