necmo commented on code in PR #65222:
URL: https://github.com/apache/airflow/pull/65222#discussion_r3104242107
##########
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
Review Comment:
Moved all inline imports to top-level in 493651c. Thanks!
##########
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:
Done in bfff051 — `__repr__`/`__str__` now delegate to `self.serialize()`,
so `repr(NOTSET) == "NOTSET"` and `SetDuringExecution` inherits the correct
polymorphic repr via its own `serialize()` override.
##########
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:
Done in 493651c — flipped the assertions and docstring to expect `"NOTSET"`
and `"DYNAMIC (set during execution)"`.
##########
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:
Applied as suggested — bfff051 delegates `__repr__`/`__str__` to
`self.serialize()`, and 493651c flips the test assertions. Thanks for the clear
writeup!
--
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]
