Copilot commented on code in PR #65222:
URL: https://github.com/apache/airflow/pull/65222#discussion_r3100662303
##########
airflow-core/src/airflow/serialization/definitions/notset.py:
##########
@@ -31,6 +31,16 @@
class ArgNotSet:
"""Sentinel type for annotations, useful when None is not viable."""
+ @staticmethod
+ def serialize() -> str:
+ return "NOTSET"
+
+ def __repr__(self) -> str:
+ return self.__class__.__name__
+
+ def __str__(self) -> str:
+ return self.__class__.__name__
Review Comment:
`__repr__`/`__str__` currently return the class name (e.g. `ArgNotSet`), but
the PR description and issue indicate we want a stable sentinel string (e.g.
`"NOTSET"`) when `str()`/`repr()` is hit in fallback serialization paths.
Consider making `__repr__`/`__str__` return `self.serialize()` (so subclasses
like `SetDuringExecution` can still return a distinct stable string via their
own `serialize()`), rather than the class name.
##########
airflow-core/tests/unit/serialization/test_helpers.py:
##########
@@ -29,3 +29,32 @@ def
test_serialize_template_field_with_very_small_max_length(monkeypatch):
# This ensures users always see why content is truncated
assert result
assert "Truncated. You can change this behaviour" in result
+
+
+def test_serialize_template_field_with_notset():
+ """NOTSET must serialize deterministically via serialize(), not str()
fallback."""
+ from airflow.serialization.definitions.notset import NOTSET
+ from airflow.serialization.helpers import serialize_template_field
+
+ result = serialize_template_field(NOTSET, "logical_date")
+ assert result == "NOTSET"
+
+
+def test_serialize_template_field_with_set_during_execution():
+ """SetDuringExecution must use its own serialize() override."""
+ from airflow.sdk.definitions._internal.types import SET_DURING_EXECUTION
+ from airflow.serialization.helpers import serialize_template_field
+
+ result = serialize_template_field(SET_DURING_EXECUTION, "logical_date")
+ assert result == "DYNAMIC (set during execution)"
+
+
+def test_argnotset_repr_and_str():
+ """repr/str should return class name, so subclasses get distinct output."""
+ from airflow.sdk.definitions._internal.types import SET_DURING_EXECUTION
+ from airflow.serialization.definitions.notset import NOTSET
+
+ assert repr(NOTSET) == "ArgNotSet"
+ assert str(NOTSET) == "ArgNotSet"
+ assert repr(SET_DURING_EXECUTION) == "SetDuringExecution"
+ assert str(SET_DURING_EXECUTION) == "SetDuringExecution"
Review Comment:
The new `test_argnotset_repr_and_str` assertions/docstring expect
`repr()`/`str()` to return the class name, which doesn’t match the PR’s stated
goal of returning a stable sentinel string like `"NOTSET"` (and would also make
`str(SET_DURING_EXECUTION)` differ from its `serialize()` output). Updating the
test (and docstring) to assert the deterministic serialized strings would
better cover the regression described in the issue.
--
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]
