Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always use Literal from typing_extensions #33794

Merged
merged 1 commit into from
Aug 27, 2023

Conversation

Taragolis
Copy link
Contributor

During the removing support of Python 3.7 #30963, we incidentally create this block

# Literal in 3.8 is limited to one single argument, not e.g. "Literal[1, 2]".
if sys.version_info >= (3, 9):
    from typing import Literal
else:
    from typing import Literal

This PR fix by use everywhere from typing_extension import Literal instead of

  • from typing import Literal
  • from airflow.typing_compat import Literal

In additional Literal has other known bugs which finally resolved in Python 3.10.1+, so personally I thought that better just use from typing_extension import Literal rather than out airflow.typing_compat resolver


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@@ -65,6 +65,7 @@
from sqlalchemy.orm.session import Session
from sqlalchemy.sql.elements import BooleanClauseList
from sqlalchemy.sql.expression import ColumnOperators, case
from typing_extensions import Literal, TypeGuard
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeGuard also presented in our airflow.typing_compat so we might want to use two imports here:

from typing_extensions import Literal

from airflow.typing_compat import TypeGuard

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@Taragolis Taragolis force-pushed the literal-from-typing-extensions branch 2 times, most recently from ed9d450 to cc1ffc2 Compare August 27, 2023 19:48
@Taragolis Taragolis force-pushed the literal-from-typing-extensions branch from cc1ffc2 to 7fec822 Compare August 27, 2023 19:57
@Taragolis
Copy link
Contributor Author

I hope I've finally finish rebasing and resolve conflicts 🤣

@hussein-awala
Copy link
Member

I hope I've finally finish rebasing and resolve conflicts 🤣

I will wait your PR before merging the other PRs

@Taragolis
Copy link
Contributor Author

No problem, that was just funny.
Anyway someone should resolve conflict - I in one PR or you in multiple

@hussein-awala
Copy link
Member

Flaky test, you can merge it if there is nothing to change.

@Taragolis Taragolis merged commit cede385 into apache:main Aug 27, 2023
64 checks passed
@Taragolis Taragolis deleted the literal-from-typing-extensions branch August 27, 2023 21:26
@uranusjr
Copy link
Member

Why is typing_extension used unconditionally instead of only for 3.10.1+ (or more conveniently just 3.11+)?

@Taragolis
Copy link
Contributor Author

Why is typing_extension used unconditionally instead of only for 3.10.1+ (or more conveniently just 3.11+)?

For provider just for do not write something this condition everywhere, seem like we save to use until we can get rid of it.

For core, maybe here we should use such condition, and I was merge before got additional review comments 😞

@uranusjr
Copy link
Member

Should I do that? (Asking in case you’re already doing it)

@Taragolis
Copy link
Contributor Author

I could do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants