uranusjr commented on code in PR #28607:
URL: https://github.com/apache/airflow/pull/28607#discussion_r1058066878
##########
airflow/models/baseoperator.py:
##########
@@ -1204,7 +1204,7 @@ def clear(
tasks += [t.task_id for t in
self.get_flat_relatives(upstream=True)]
if downstream:
- tasks += [t.task_id for t in
self.get_flat_relatives(upstream=False)]
+ tasks += [t.task_id for t in self.get_flat_relatives()]
Review Comment:
The default value of `upstream` is False, and we need to keep it for
backward compatibility, but I’d prefer to keep it at the call sites for
explicitness.
##########
airflow/providers/ssh/operators/ssh.py:
##########
@@ -63,7 +63,10 @@ class SSHOperator(BaseOperator):
"""
template_fields: Sequence[str] = ("command", "environment", "remote_host")
- template_ext: Sequence[str] = (".sh",)
+ template_ext: Sequence[str] = (
+ ".sh",
+ ".bash",
+ )
Review Comment:
This looks unrelated? I’m not against its addition, but it should be done in
a separate PR.
##########
airflow/models/baseoperator.py:
##########
@@ -413,7 +413,7 @@ def apply_defaults(self: BaseOperator, *args: Any,
**kwargs: Any) -> Any:
def __new__(cls, name, bases, namespace, **kwargs):
new_cls = super().__new__(cls, name, bases, namespace, **kwargs)
with contextlib.suppress(KeyError):
- # Update the partial descriptor with the class method so it call
call the actual function (but let
+ # Update the partial descriptor with the class method, so it call
the actual function (but let
Review Comment:
```suggestion
# Update the partial descriptor with the class method, so it
calls the actual function (but let
```
##########
scripts/ci/testing/summarize_junit_failures.py:
##########
@@ -32,45 +32,62 @@
@lru_cache(maxsize=None)
-def translate_classname(classname):
- # The pytest xunit output has "classname" in the python sense as
'dir.subdir.package.Class' -- we want to
- # convert that back in to a pytest "selector" of
dir/subdir/package.py::Class
+def translate_classname(classname: str) -> str | None:
+ """
+ Translates a classname in the form 'dir.subdir.package.Class' to a pytest
selector in the form
+ 'dir/subdir/package.py::Class'.
+
+ Args:
+ classname: The classname to translate.
+
+ Returns:
+ The translated pytest selector.
Review Comment:
We generally use Sphinx-style derivatives instead of Google-style. See other
functions for examples.
##########
scripts/ci/testing/summarize_junit_failures.py:
##########
@@ -32,45 +32,62 @@
@lru_cache(maxsize=None)
-def translate_classname(classname):
- # The pytest xunit output has "classname" in the python sense as
'dir.subdir.package.Class' -- we want to
- # convert that back in to a pytest "selector" of
dir/subdir/package.py::Class
+def translate_classname(classname: str) -> str | None:
+ """
+ Translates a classname in the form 'dir.subdir.package.Class' to a pytest
selector in the form
+ 'dir/subdir/package.py::Class'.
+
+ Args:
+ classname: The classname to translate.
+
+ Returns:
+ The translated pytest selector.
+ """
if not classname:
return None
- context = Path.cwd()
-
+ context = str(Path.cwd())
parts = classname.split(".")
+ num_parts = len(parts)
+ i = 0
- for i, component in enumerate(parts):
- candidate = context / component
+ while i < num_parts:
+ component = parts[i]
+ candidate = context + "/" + component
- if candidate.is_dir():
- context = candidate
- continue
- candidate = context / (component + ".py")
- if candidate.is_file():
+ if Path(candidate).is_dir():
context = candidate
i += 1
+ elif Path(candidate + ".py").is_file():
+ context = candidate + ".py"
+ i += 1
break
- break
+ else:
+ break
+
parts = parts[i:]
- val = str(context.relative_to(Path.cwd()))
+ val = str(Path(context).relative_to(Path.cwd()))
Review Comment:
Why is this particular function rewritten so extensively? This should be
done in a different PR, and honestly I’m not sure the rewrite is an improvement
to begine with.
--
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]
