[GitHub] [airflow] pgagnon commented on a change in pull request #5372: [AIRFLOW-3057] add prev_*_date_success to template context

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

pgagnon commented on a change in pull request #5372: [AIRFLOW-3057] add prev_*_date_success to template context
URL: https://github.com/apache/airflow/pull/5372#discussion_r291154674
 
 

 ##########
 File path: airflow/models/dagrun.py
 ##########
 @@ -218,12 +219,19 @@ def get_dag(self):
         return self.dag
 
     @provide_session
-    def get_previous_dagrun(self, session=None):
+    def get_previous_dagrun(self, session: Session = None, state: str = None) -> Optional['DagRun']:
         """The previous DagRun, if there is one"""
 
-        return session.query(DagRun).filter(
+        assert isinstance(session, Session)  # mypy
 
 Review comment:
   @dstandish By default mypy treats arguments which default to `None` as implicit `Optional[T]`. IMO this is a good thing because it adds a minimal null-safety threshold to typed Python, but it creates these confusing situations.
   
   If as @ashb asserts the decorator will _always_ pass a `Session` object I would just remove the `isinstance` call and `None` default value to the `session` argument.
   
   As a side note, there are situations where you positively need an argument with defaults to `None`, but where you know for a fact that in practice this will never happen (i.e. some usages of `functools.partial`) In these cases you could use a mypy `cast` here instead of `isinstance`. The difference being that `cast` is a dud at runtime.

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


With regards,
Apache Git Services