Copilot commented on code in PR #65222:
URL: https://github.com/apache/airflow/pull/65222#discussion_r3089785199
##########
airflow-core/src/airflow/serialization/definitions/notset.py:
##########
@@ -31,6 +31,15 @@
class ArgNotSet:
"""Sentinel type for annotations, useful when None is not viable."""
+ def __repr__(self) -> str:
+ return "NOTSET"
+
+ def __str__(self) -> str:
+ return "NOTSET"
Review Comment:
`__repr__`/`__str__` currently return the hard-coded string "NOTSET". Since
`ArgNotSet` is subclassed in the Task SDK (e.g. `SetDuringExecution` overrides
`serialize()` to return "DYNAMIC (set during execution)"), this change will
cause `str()`/`repr()` on those subclasses to incorrectly show "NOTSET".
Consider delegating `__repr__`/`__str__` to `self.serialize()` (or otherwise
making the representation polymorphic) so subclasses keep their distinct,
stable string forms.
```suggestion
return self.serialize()
def __str__(self) -> str:
return self.serialize()
```
##########
airflow-core/src/airflow/serialization/definitions/notset.py:
##########
@@ -31,6 +31,15 @@
class ArgNotSet:
"""Sentinel type for annotations, useful when None is not viable."""
+ def __repr__(self) -> str:
+ return "NOTSET"
+
+ def __str__(self) -> str:
+ return "NOTSET"
+
+ def serialize(self) -> str:
+ return "NOTSET"
Review Comment:
This introduces new public behavior for `ArgNotSet` (`__repr__`, `__str__`,
and `serialize()`), but there is no regression test ensuring these values are
stable/deterministic (and that subclasses, like `SetDuringExecution`, keep
distinct output). Adding a focused unit test would help prevent reintroducing
non-deterministic serialized DAG diffs and protect the string/serialization
contract going forward.
--
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]
