Skip to content

Ban import AirflowException from airflow - #34512

Merged
Taragolis merged 1 commit into
apache:mainfrom
Taragolis:ban-airflow-exception
Sep 21, 2023
Merged

Ban import AirflowException from airflow#34512
Taragolis merged 1 commit into
apache:mainfrom
Taragolis:ban-airflow-exception

Conversation

@Taragolis

Copy link
Copy Markdown
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.

@hussein-awalahussein-awala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice one!

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

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

I think we also could do that. I think this "shortcuts" valuable for the endusers rather than in airflow codebase.

Comment threadpyproject.toml
"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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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?

@ephraimbuddyephraimbuddySep 21, 2023

Copy link
Copy Markdown
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:mainSep 21, 2023
@Taragolis
Taragolis deleted the ban-airflow-exception branch September 21, 2023 22:24
@ephraimbuddyephraimbuddy added this to the Airflow 2.8.0 milestone Oct 3, 2023
@ephraimbuddyephraimbuddy added the type:improvement Changelog: Improvements label Oct 3, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full tests neededWe need to run full set of tests for this PR to mergetype:improvementChangelog: Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Taragolis@vincbeck@ephraimbuddy@Lee-W@hussein-awala