dstandish commented on PR #28019: URL: https://github.com/apache/airflow/pull/28019#issuecomment-1334608169
> we want to use assert for if TYPE_CHECKING when we want to get rid of mypy warning that None has no property etc Yes, and by my reading, it appears that's exactly what these asserts are there for. Essentially what's required is that executor.start is called before the executor is used. And, it looks like that's in fact what happens. It looks like you added these initially [here](https://github.com/apache/airflow/commit/a36cfe049a2c5948b24fde7a878fe19cabede5f7). Presumably, prior to that change, mypy never complained because the attrs were not defined and annotated in the init method -- they were created in `start`. But in your change you initialized them as None and annotated them, e.g. [here](https://github.com/apache/airflow/blob/a36cfe049a2c5948b24fde7a878fe19cabede5f7/airflow/executors/dask_executor.py#L45). I presume that this is why the asserts had to be added (e.g. [here](https://github.com/apache/airflow/blob/a36cfe049a2c5948b24fde7a878fe19cabede5f7/airflow/executors/dask_executor.py#L66)) -- to get mypy to not complain. That PR I assume predated any discussions about asserts vs raising. Then when it was decided to go with raising, except in the case of mypy appeasement, these asserts were changed. But, it does seem that these are mypy appeasement and could therefore have been left alone, or put behind a type checking guard, as I've proposed. In any case, if you see it another way, that's fine and you're welcome to close this. -- 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]
