Mypy fixes to DagRun, TaskInstance, and db utils #20163
Conversation
| models_to_dagrun = [TaskInstance, TaskReschedule] | ||
| models_to_dagrun: List[Any] = [TaskInstance, TaskReschedule] |
There was a problem hiding this comment.
Technically we can type this (what we want is the __tablename__ attribute from SQLAlchemy), but SQLAlchemy is so dynamic, adding accurate type here does not really help much, so I don’t really want to bother.
There was a problem hiding this comment.
👍🏻 Also, as this isn't called anywhere from outside of this module it doesn't impact the typing of the rest of the code base.
| self, state: Optional[Iterable[TaskInstanceState]] = None, session=None | ||
| self, | ||
| state: Optional[Iterable[Optional[TaskInstanceState]]] = None, | ||
| session: Session = NEW_SESSION, |
There was a problem hiding this comment.
In a separate PR or this (your choice) - can you add a small section in https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices to let devs know when they should use NEW_SESSION vs Session for type hints
github-actions
bot
added
the
full tests needed
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
kaxil
removed
the
full tests needed
| def get_task_instances( | ||
| self, state: Optional[Iterable[TaskInstanceState]] = None, session=None | ||
| self, | ||
| state: Optional[Iterable[Optional[TaskInstanceState]]] = None, |
There was a problem hiding this comment.
Isn't None a valid TaskInstanceState?
airflow/airflow/utils/state.py
Line 35 in 820e836
There was a problem hiding this comment.
I suspect subclassing from str is the problem here (but we probably can’t fix that without breaking a ton of things).
github-actions
bot
added
the
full tests needed
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
kaxil
added
the
changelog:skip
4 participants
Some more typing fixups found when implementing another feature, submitted separately.