[GitHub] [airflow] uranusjr commented on a change in pull request #15397: AIP-39: Use TimeTable interface to implement scheduling inside the DAG class

[GitBox <gi...@apache.org>]

uranusjr commented on a change in pull request #15397:
URL: https://github.com/apache/airflow/pull/15397#discussion_r617133073



##########
File path: airflow/models/dag.py
##########
@@ -109,6 +112,21 @@ def get_last_dagrun(dag_id, session, include_externally_triggered=False):
     return query.first()
 
 
+def coerce_datetime(v: Union[None, datetime, pendulum.DateTime]) -> Optional[pendulum.DateTime]:
+    """Convert whatever is passed in to ``pendulum.DateTime``.
+
+    This is for interfacing with the new ``timetables`` package, which
+    exclusively uses ``pendulum.DateTime`` internally.
+    """
+    if v is None:
+        return None
+    if isinstance(v, pendulum.DateTime):
+        return v
+    if v.tzinfo is None:
+        v = timezone.make_aware(v)
+    return pendulum.instance(v)

Review comment:
       My hope is actually to eventually remove this function altogether. If you looks at the annotation of `DAG.next_dagrun_info()`, the function has every intention to expect only `Optional[pendulum.DateTime]`, not `datetime.datetime`:
   
   https://github.com/apache/airflow/blob/b314c7168fbf8707c7b438b8f912ab5ed63b82ea/airflow/models/dag.py#L524-L527
   
   But someone somewhere passes in an incorrect value and breaks the promise. So that someone should be fixed instead.
   
   With that said, it’s probably not unreasonable to put the function in `airflow.utils.timezone` anyway.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org