Lee-W commented on code in PR #57700:
URL: https://github.com/apache/airflow/pull/57700#discussion_r2501477294
##########
airflow-core/src/airflow/serialization/serde.py:
##########
@@ -365,30 +365,29 @@ def _register():
_stringifiers.clear()
with Stats.timer("serde.load_serializers") as timer:
- for _, name, _ in iter_namespace(airflow.serialization.serializers):
- name = import_module(name)
- for s in getattr(name, "serializers", ()):
- if not isinstance(s, str):
- s = qualname(s)
- if s in _serializers and _serializers[s] != name:
- raise AttributeError(f"duplicate {s} for serialization in
{name} and {_serializers[s]}")
+ 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)
Review Comment:
```suggestion
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 🤔
##########
airflow-core/src/airflow/executors/executor_loader.py:
##########
@@ -74,11 +74,11 @@ def _get_executor_names(cls) -> list[ExecutorName]:
all_executor_names: list[tuple[str | None, list[str]]] =
cls._get_team_executor_configs()
- executor_names = []
+ executor_names: list[ExecutorName] = []
for team_name, executor_names_config in all_executor_names:
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:
Review Comment:
do we need the `_str` here? ignore me if it's needed 👀
```suggestion
for executor_name in executor_names_config:
```
##########
airflow-core/src/airflow/serialization/serde.py:
##########
@@ -365,30 +365,29 @@ def _register():
_stringifiers.clear()
with Stats.timer("serde.load_serializers") as timer:
- for _, name, _ in iter_namespace(airflow.serialization.serializers):
- name = import_module(name)
- for s in getattr(name, "serializers", ()):
- if not isinstance(s, str):
- s = qualname(s)
- if s in _serializers and _serializers[s] != name:
- raise AttributeError(f"duplicate {s} for serialization in
{name} and {_serializers[s]}")
+ 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)
+ if s in _serializers and _serializers[s] != module:
+ raise AttributeError(f"duplicate {s} for serialization in
{module} and {_serializers[s]}")
log.debug("registering %s for serialization", s)
- _serializers[s] = name
- for d in getattr(name, "deserializers", ()):
- if not isinstance(d, str):
- d = qualname(d)
- if d in _deserializers and _deserializers[d] != name:
- raise AttributeError(f"duplicate {d} for deserialization
in {name} and {_serializers[d]}")
+ _serializers[s] = module
+ for deserializers in getattr(module, "deserializers", ()):
+ d = deserializers if isinstance(deserializers, str) else
qualname(deserializers)
+ if d in _deserializers and _deserializers[d] != module:
+ raise AttributeError(
+ f"duplicate {d} for deserialization in {module} and
{_deserializers[d]}"
+ )
log.debug("registering %s for deserialization", d)
- _deserializers[d] = name
+ _deserializers[d] = module
_extra_allowed.add(d)
- for c in getattr(name, "stringifiers", ()):
- if not isinstance(c, str):
- 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", ()):
Review Comment:
Probably not something we need to do it this PR. but we maybe can duplicate
these few for loop 🤔
##########
airflow-core/tests/unit/plugins/test_plugin_ignore.py:
##########
@@ -104,8 +104,8 @@ def test_find_not_should_ignore_path_glob(self, tmp_path):
}
ignore_list_file = ".airflowignore_glob"
print("-" * 20)
- for file_path in find_path_from_directory(plugin_folder_path,
ignore_list_file, "glob"):
- file_path = Path(file_path)
+ for file_path_raw in find_path_from_directory(plugin_folder_path,
ignore_list_file, "glob"):
+ file_path = Path(file_path_raw)
Review Comment:
```suggestion
for raw_file_path in find_path_from_directory(plugin_folder_path,
ignore_list_file, "glob"):
file_path = Path(raw_file_path)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
