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

Rename TaskMixin to DependencyMixin #20297

Merged
merged 4 commits into from
Dec 15, 2021
Merged

Conversation

ashb
Copy link
Member

@ashb ashb commented Dec 14, 2021

It was used on things that weren't tasks (such as XComArg and
EdgeModifier) so the name was in-correct, and I want to disambiguate
this from a concept I need to add for AIP-42 (Dynamic task mapping) of
things that actually are Tasks or Task-like objects.

Since Union[TaskMixin, Sequence[TaskMixin]] was uses in so many places
I created a type alias for this of DependencyMixinOrList


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@ashb ashb requested review from kaxil and XD-DENG as code owners December 14, 2021 19:11
@ashb ashb requested a review from uranusjr December 14, 2021 19:11
airflow/models/taskmixin.py Outdated Show resolved Hide resolved
ashb added 2 commits December 14, 2021 20:57
It was used on things that weren't tasks (such as XComArg and
EdgeModifier) so the name was in-correct, and I want to disambiguate
this from a concept I need to add for AIP-42 (Dynamic task mapping) of
things that actually _are_ Tasks or Task-like objects.

Since `Union[TaskMixin, Sequence[TaskMixin]]` was uses in so many places
I created a type alias for this of `DependencyMixinOrList`
Comment on lines +1663 to +1664
# TODO: Deprecate for Airflow 3.0
Chainable = Union[DependencyMixin, Sequence[DependencyMixin]]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it’s actually a good idea to keep using this name…? It’s unfortunately not super easy to deprecate a type alias.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this alias is used anywhere beyond the usage a few lines below.

@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 15, 2021
@uranusjr
Copy link
Member

Also need to add DependencyMixin to spellings I think.

@ashb
Copy link
Member Author

ashb commented Dec 15, 2021

Also need to add DependencyMixin to spellings I think.

Yeah, I didn't get that locally. I wonder why.

(Also I wish sphinxcontrib-spelling was clever enough to ignore class and module names from the code base)

@ashb ashb merged commit bc9ca83 into apache:main Dec 15, 2021
@ashb ashb deleted the rename-taskmixin branch December 15, 2021 14:26
@uranusjr
Copy link
Member

Also I wish sphinxcontrib-spelling was clever enough to ignore class and module names from the code base

I suspect it is for sphinx.ext.autodoc, but sphinx-autoapi’s output is different.

@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Apr 14, 2022
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:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants