ExternalTaskSensor. execution-date pattern.#67138
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
b56aa7f to
53a99f5
Compare
| if isinstance(execution_date_value, datetime.datetime): | ||
| return [execution_date_value] | ||
|
|
||
| import pendulum |
There was a problem hiding this comment.
Thank you for your contribution.
I observed the following points:
I believe that lazy importing in this context doesn't make much sense; the gain is low and it deviates from the file's standard. After all, _get_dttm_filter runs on every poke/defer.
Another problem related to the pendulum: I noticed that we can import timezone through the SDK, and we even have classes imported from it in this file. It's worth evaluating if this makes sense; it would be one less library in the file.
Here's a possible suggestion regarding parsing using airflow.providers.common.compat.sdk. Some operators already use this pattern.
Therefore, we could replace:
import pendulum
...
return [pendulum.parse(execution_date_value)]Put:
from airflow.providers.common.compat.sdk import (
AirflowSkipException,
BaseOperatorLink,
BaseSensorOperator,
conf,
timezone,
)
...
return [timezone.parse(execution_date_value)]There was a problem hiding this comment.
Thanks a lot. I will change the code soon. Have one test to fix 😓
There was a problem hiding this comment.
done. The tests should be green as well
There was a problem hiding this comment.
The pendulum removal is perfect, thank you.
53a99f5 to
120e55c
Compare
Pedrinhonitz
left a comment
There was a problem hiding this comment.
It seems good to me, but I would like a more in-depth analysis from a more experienced colleague to assess the impacts of depreciation.
Fixes #10883.
Implemented execution_date pattern.
allows for the following: