kaxil commented on code in PR #65222:
URL: https://github.com/apache/airflow/pull/65222#discussion_r3089804862
##########
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:
Review Comment:
`SetDuringExecution` in
`task-sdk/src/airflow/sdk/definitions/_internal/types.py` inherits `__repr__`
and `__str__` from `ArgNotSet` but doesn't override them, so
`repr(SET_DURING_EXECUTION)` would return `"NOTSET"` instead of identifying
itself correctly.
Suggestion: use `return self.__class__.__name__` instead of hardcoding
`"NOTSET"` -- that way `SetDuringExecution` automatically gets the right repr
without needing its own override.
##########
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:
Review Comment:
Two things:
1. This doesn't use `self`. `SetDuringExecution.serialize()` in the Task SDK
is a `@staticmethod`, so this should be too for consistency.
2. Could you add a test for the actual bug path? Something like `assert
serialize_template_field(NOTSET, "logical_date") == "NOTSET"` would verify the
fix end-to-end (the leak is in `helpers.py` line 78 where `.serialize()` is
tried before falling to `str()`).
--
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]
