potiuk commented on code in PR #70640:
URL: https://github.com/apache/airflow/pull/70640#discussion_r3682235286
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -223,8 +223,8 @@ def __init_subclass__(cls, **kwargs):
f = cls._redact
@functools.wraps(f)
- def _redact(*args, replacement: str = "***", **kwargs):
- return f(*args, **kwargs)
+ def _redact(*args, replacement: str = "***", _f=f, **kwargs):
Review Comment:
Binding via a default argument leaks `_f` into the function's **keyword
signature**, and this one is wrapped in `functools.wraps` — so introspection
now advertises a parameter that isn't part of the contract, and a caller who
happens to pass `_f` in `**kwargs` would silently substitute a different
function. For a secrets-masking code path that's a sharper edge than it looks.
A closure factory avoids exposing anything:
```python
def _make_redact(f):
@functools.wraps(f)
def _redact(*args, replacement: str = "***", **kwargs):
return f(*args, **kwargs)
return _redact
```
Same effect, nothing added to the signature.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/migrations/versions/0101_3_2_0_ui_improvements_for_deadlines.py:
##########
@@ -594,7 +594,13 @@ def _migrate_deadline_alerts() -> None:
dags_with_deadlines.add(dag_id)
deadline_alerts = dag_deadline if isinstance(dag_deadline, list)
else [dag_deadline]
- def _migrate_dag_deadlines(dag_conn: Connection) -> Iterable[str]:
+ def _migrate_dag_deadlines(
Review Comment:
Please revert this file and use `# noqa: B023` instead.
This is a **released migration** (0101, shipped in 3.2.0) and the change is
semantically a no-op — the closure is called at line 688 inside the same
iteration where it's defined here, so no value ever leaked across iterations.
Rewriting a shipped migration to satisfy a linter means a user who already
ran it has different source than a fresh install, for zero behavioural gain. A
`# noqa: B023` with a one-line comment ("closure is invoked within the same
iteration") leaves the historical file intact and documents *why* it's safe,
which is more useful to the next reader than five extra default arguments.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
