vincbeck commented on code in PR #45562:
URL: https://github.com/apache/airflow/pull/45562#discussion_r1918886705
##########
airflow/triggers/base.py:
##########
@@ -124,6 +124,11 @@ def __repr__(self) -> str:
classpath, kwargs = self.serialize()
return self.repr(classpath, kwargs)
+ def __eq__(self, other: Any) -> bool:
Review Comment:
That's for a test. Without it, the test fails, I dont remember which one
though. I'll remove it to find out. We can see from there if it should be
addressed that way
##########
airflow/serialization/serialized_objects.py:
##########
@@ -297,6 +306,25 @@ def decode_asset_condition(var: dict[str, Any]) ->
BaseAsset:
raise ValueError(f"deserialization not implemented for DAT {dat!r}")
+def decode_asset(var: dict[str, Any]) -> BaseAsset:
+ """
+ Decode a previously serialized asset.
+
+ :meta private:
+ """
+
+ def _decode_trigger(trigger_infos: dict[str, Any]) -> BaseTrigger:
+ return
import_string(trigger_infos["classpath"])(**trigger_infos["kwargs"])
Review Comment:
Thanks a lot for the thorough review! You are right, we do not need to
create the trigger object. I was creating the trigger object to be consistent
whether the source was coming from the DAG or the DB (serialized DAG). But as
you pointed out, we can handle the 2 use cases differently and that way no need
to create the trigger object
##########
tests/timetables/test_assets_timetable.py:
##########
@@ -142,6 +142,7 @@ def test_serialization(asset_timetable:
AssetOrTimeSchedule, monkeypatch: Any) -
"uri": "test://asset/",
"group": "asset",
"extra": {},
+ "watchers": [],
Review Comment:
That makes sense! I removed it if there is no watcher
--
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]
