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

Ban import AirflowException from airflow #34512

Merged
merged 1 commit into from Sep 21, 2023

Conversation

Taragolis
Copy link
Contributor

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.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Nice one!

@Taragolis Taragolis added the full tests needed We need to run full set of tests for this PR to merge label Sep 21, 2023
@Taragolis Taragolis closed this Sep 21, 2023
@Taragolis Taragolis reopened this Sep 21, 2023
@Taragolis Taragolis marked this pull request as ready for review September 21, 2023 09:01
@vincbeck
Copy link
Contributor

Nice! Should not we do the same with models? e.g. from airflow.models.dagrun import DagRun instead of from airflow.models import DagRun

@Taragolis
Copy link
Contributor Author

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."
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this

"AirflowException": (".exceptions", "AirflowException"),
then?

Copy link
Contributor Author

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__

Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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

@Taragolis Taragolis merged commit e27325a into apache:main Sep 21, 2023
65 checks passed
@Taragolis Taragolis deleted the ban-airflow-exception branch September 21, 2023 22:24
@ephraimbuddy ephraimbuddy added this to the Airflow 2.8.0 milestone Oct 3, 2023
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants