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
Ban import AirflowException from airflow #34512
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one!
72a9e87
to
7b1da83
Compare
Nice! Should not we do the same with models? e.g. |
7b1da83
to
d4b69b8
Compare
I think we also could do that. I think this "shortcuts" valuable for the endusers rather than in airflow codebase. |
@@ -144,6 +145,9 @@ combine-as-imports = true | |||
"tests/providers/elasticsearch/log/elasticmock/__init__.py" = ["E402"] | |||
"tests/providers/elasticsearch/log/elasticmock/utilities/__init__.py" = ["E402"] | |||
|
|||
[tool.ruff.flake8-tidy-imports.banned-api] | |||
"airflow.AirflowException".msg = "Use airflow.exceptions.AirflowException instead." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this
Line 84 in 117e404
"AirflowException": (".exceptions", "AirflowException"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also thought about it and I remembered that I used this import myself from the first days with Airflow (1.10.4 will remain in my heart forever) as DAD Author.
Better what we could do it deprecate it because in general this import is useless, even for end-users the reason is simple all other exceptions such as AirflowSkipException
, AirflowFailException
do-not lazy loaded in airflow.__init__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'm happy with deprecation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could deprecate it in this PR or in the separate one. Which option is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate one will be better
related: #34510 and #34511
^ 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.