Enable ruff PLW2101,PLW2901,PLW3301 rule #57700

jscheffl · 2025-11-01T21:45:09Z

While implementing #57294 I realized that a couple of more ruff PLW rules might be interesting, this PR enables

See also https://docs.astral.sh/ruff/rules/#warning-plw

pierrejeambrun

PLW2901 seems good to me, I find it to be a source of error and confusion too.

pyproject.toml

uranusjr · 2025-11-05T02:08:07Z

airflow-core/src/airflow/serialization/serialized_objects.py

+            else:
+                v = v_in


This one is a -0 from me since they arguably not only don’t help, but acually make this kind of operations easier to get wrong.

Hi @uranusjr are you referring to this specific case of serialized objects (where we could exclude with # noqa: ... or is it a general concern?

In rework making the codebase compatible I also had one other point in https://github.com/apache/airflow/pull/57700/files#diff-94532ce5cd8778da565396e51f54f7a10078725a1f61656615fbb173b7503fceR1818 where making code compliant made me a hard time.

So do you mean in these specific cases we should exclude rule where it makes code not better? Or do you dislike the rule in general (I think in 90% of cases it prevents failures).

I can also raise a discussion in devlist if you think we should have a larger round of forming an opinion on the rule.

It’s -0 so I’d not block this, but personally I’m not into this loop variable reuse rule in general.

I reverted the usage in serialized_objects and made it minimal invasive. Hope this improves to have a 0.000001 instead of -0 :-D

Lee-W · 2025-11-06T03:54:38Z

the descirption was inaccurate, just updated it.

airflow-core/src/airflow/api_fastapi/core_api/services/public/variables.py

airflow-core/src/airflow/dag_processing/collection.py

airflow-core/src/airflow/executors/executor_loader.py

airflow-core/src/airflow/models/pool.py

airflow-core/src/airflow/serialization/serde.py

airflow-core/tests/unit/core/test_impersonation_tests.py

airflow-core/tests/unit/plugins/test_plugin_ignore.py

providers/amazon/src/airflow/providers/amazon/aws/hooks/athena.py

pierrejeambrun

Wei suggestions are good.

Beside naming the general approach sounds good to me.

jscheffl · 2025-11-06T20:33:49Z

the descirption was inaccurate, just updated it.

Thanks @Lee-W for the great feedback! Adjusted names to your very good proposals!

Lee-W

A few nits. But looks great! Thanks!

Lee-W · 2025-11-07T02:28:40Z

airflow-core/src/airflow/executors/executor_loader.py

            executor_names_per_team = []
-            for name in executor_names_config:
-                if len(split_name := name.split(":")) == 1:
+            for executor_name_str in executor_names_config:


do we need the _str here? ignore me if it's needed 👀

Suggested change

for executor_name_str in executor_names_config:

for executor_name in executor_names_config:

Lee-W · 2025-11-07T02:30:28Z

airflow-core/src/airflow/serialization/serde.py

+        for _, module_name, _ in iter_namespace(airflow.serialization.serializers):
+            module = import_module(module_name)
+            for serializers in getattr(module, "serializers", ()):
+                s = serializers if isinstance(serializers, str) else qualname(serializers)


Suggested change

s = serializers if isinstance(serializers, str) else qualname(serializers)

serializers_qualname = serializers if isinstance(serializers, str) else qualname(serializers)

nit: s or d don't fit my taste. or s_qualname might be better 🤔

Lee-W · 2025-11-07T02:31:20Z

airflow-core/src/airflow/serialization/serde.py

-                    c = qualname(c)
-                if c in _deserializers and _deserializers[c] != name:
-                    raise AttributeError(f"duplicate {c} for stringifiers in {name} and {_stringifiers[c]}")
+            for stringifiers in getattr(module, "stringifiers", ()):


Probably not something we need to do it this PR. but we maybe can duplicate these few for loop 🤔

Lee-W · 2025-11-07T02:31:51Z

airflow-core/tests/unit/plugins/test_plugin_ignore.py

+        for file_path_raw in find_path_from_directory(plugin_folder_path, ignore_list_file, "glob"):
+            file_path = Path(file_path_raw)


Suggested change

for file_path_raw in find_path_from_directory(plugin_folder_path, ignore_list_file, "glob"):

file_path = Path(file_path_raw)

for raw_file_path in find_path_from_directory(plugin_folder_path, ignore_list_file, "glob"):

file_path = Path(raw_file_path)

boring-cyborg bot added area:API Airflow's REST/HTTP API area:CLI area:DAG-processing area:dev-tools area:Executors-core LocalExecutor & SequentialExecutor area:helm-chart Airflow Helm Chart area:plugins area:providers area:secrets area:serialization labels Nov 1, 2025

boring-cyborg bot added backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch kind:documentation provider:amazon AWS/Amazon - related issues labels Nov 1, 2025

jscheffl force-pushed the feature/add-ruff-plw2901-rule branch from 6b58278 to 0eb1c69 Compare November 2, 2025 14:14

pierrejeambrun reviewed Nov 4, 2025

View reviewed changes

pyproject.toml Show resolved Hide resolved

jscheffl changed the title ~~Enable ruff PLW2901 rule~~ Enable ruff PLW2101,PLW2901,PLW3301 rule Nov 4, 2025

jscheffl force-pushed the feature/add-ruff-plw2901-rule branch from c408eb3 to 0b47076 Compare November 4, 2025 22:43

jscheffl requested a review from pierrejeambrun November 4, 2025 22:43

uranusjr reviewed Nov 5, 2025

View reviewed changes

Lee-W reviewed Nov 6, 2025

View reviewed changes

pierrejeambrun approved these changes Nov 6, 2025

View reviewed changes

jscheffl added 8 commits November 6, 2025 21:22

Enable ruff PLW2901 rule

71aa702

Fix follow-up mypy complaints after PLW2901 rule

a79ba94

Fix tests for pools and infinite size

ebbe31b

Fix logic in bigquery

3ef7332

Fix variable renaming

2f1fd6f

Fix mypy alert

d5a04ba

Review feedback by Wei

235f703

Fix mypy

a06cd94

jscheffl force-pushed the feature/add-ruff-plw2901-rule branch from 0b47076 to a06cd94 Compare November 6, 2025 20:28

Revert accidentially deleted line

c9e3a85

Lee-W approved these changes Nov 7, 2025

View reviewed changes

	for executor_name_str in executor_names_config:
	for executor_name in executor_names_config:

	s = serializers if isinstance(serializers, str) else qualname(serializers)
	serializers_qualname = serializers if isinstance(serializers, str) else qualname(serializers)

		for file_path_raw in find_path_from_directory(plugin_folder_path, ignore_list_file, "glob"):
		file_path = Path(file_path_raw)

Enable ruff PLW2101,PLW2901,PLW3301 rule #57700

Are you sure you want to change the base?

Enable ruff PLW2101,PLW2901,PLW3301 rule #57700

Conversation

jscheffl commented Nov 1, 2025 • edited by Lee-W Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierrejeambrun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lee-W commented Nov 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pierrejeambrun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jscheffl commented Nov 6, 2025

Uh oh!

Lee-W left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jscheffl commented Nov 1, 2025 •

edited by Lee-W

Loading

pierrejeambrun left a comment •

edited

Loading