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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org