Add compat module for typing execute context in operators
#770
Conversation
josh-fell
requested review from
dimberman,
tatiana,
utkarsharma2,
sunank200,
pankajastro,
pankajkoti and
feluelle
as code owners
|
|
||
| from astro.files.base import File | ||
| from astro.files.locations import create_file_location | ||
|
|
||
| if TYPE_CHECKING: |
There was a problem hiding this comment.
same as astronomer/astronomer-providers#619 (comment)
thoughts?
There was a problem hiding this comment.
@josh-fell & @kaxil I like @kaxil 's proposal of having a compatibility module.
It would also be nice to see tests covering this issue - since we are using nox, it should be straightforward to add a test.
There was a problem hiding this comment.
Absolutely, great idea! I'll add a compat module and unify the type across the repo, where applicable. And some tests too.
There was a problem hiding this comment.
Actually, any preference on having a pre-commit hook to check that the typing compat module is used vs. a unit test?
| @@ -18,6 +17,10 @@ | |||
| from astro.sql.operators.load_file import LoadFileOperator | |||
| from astro.sql.table import Table | |||
|
|
|||
| if TYPE_CHECKING: | |||
| # TODO: This can be removed from TYPE_CHECKING once there is a minimum requirement of Airflow 2.2.3+ | |||
| from airflow.utils.context import Context | |||
There was a problem hiding this comment.
Let's make sure to change to "Context" in all places of the current file as you have done it for the other file.
|
Added a pre-commit hook along with a |
josh-fell
changed the title
airflow.utils.context.Context import under TYPE_CHECKINGcontext in operators
kaxil
added
the
safe to test
|
Actually, since this repo implements PEP563 typing evaluations, we could still use Just another option to consider and I'll defer to the group on maintenance preference. Trivial updates either way I suppose. |
The `airflow.utils.context` module was not introduced to OSS Airflow until 2.2.3. Importing this module as a top-level import sets an implicit, minimum requirement for Airflow 2.2.3 which is higher than the minumum apache-airflow requirement set for Astro SDK.
for more information, see https://pre-commit.ci
Closes: #769
Description
What is the current behavior?
The
airflow.utils.contextmodule was not introduced to OSS Airflow until 2.2.3. Importing this module as a top-level import sets an implicit, minimum requirement for Airflow 2.2.3 which is higher than the minimumapache-airflowrequirement set for Astro SDK.What is the new behavior?
contextin operators.Does this introduce a breaking change?
It should not.
Checklist