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

Remove found_descendents param from get_flat_relative_ids #31559

Merged
merged 5 commits into from
May 30, 2023

Conversation

dstandish
Copy link
Contributor

By the looks of it, this param is unused. Since the class is designated private, it is permissable to remove it.

dstandish added 4 commits May 25, 2023 14:31
By the looks of it, this param is unused.  Since the class is designated private, it is permissable to remove it.
@dstandish dstandish marked this pull request as ready for review May 25, 2023 22:03
@dstandish dstandish requested a review from uranusjr as a code owner May 25, 2023 22:03
def get_flat_relative_ids(
self,
upstream: bool = False,
found_descendants: set[str] | None = None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this param here

Comment on lines -167 to -168
if found_descendants is None:
found_descendants = set()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thus no longer need this conditional

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

This is technically breaking backward compatibility (the user can technically access this function is DAGs) but I’m willing to take the chance and assume no-one is actually using it.

@dstandish
Copy link
Contributor Author

This is technically breaking backward compatibility (the user can technically access this function is DAGs) but I’m willing to take the chance and assume no-one is actually using it.

@uranusjr but then what does meta private mean in the class? i thought we took that to mean thou shalt not depend on anything in the class and we may change at any time?

@potiuk
Copy link
Member

potiuk commented May 27, 2023

This is technically breaking backward compatibility (the user can technically access this function is DAGs) but I’m willing to take the chance and assume no-one is actually using it.

@uranusjr but then what does meta private mean in the class? i thought we took that to mean thou shalt not depend on anything in the class and we may change at any time?

Yes. I think :meta private: works as designed.
And we actually have now a very good way to assess if something is breaking.

https://airflow.apache.org/docs/apache-airflow/stable/public-airflow-interface.html

Adding the page was one of the reasons to make such decisions quiickly

That technicall means it's not part of the public interface and we are free to change it.

airflow/models/abstractoperator.py Show resolved Hide resolved
@dstandish dstandish merged commit 0cbc0dc into apache:main May 30, 2023
@dstandish dstandish deleted the remove-found-descendents-param branch May 30, 2023 16:02
@eladkal eladkal added this to the Airflow 2.6.2 milestone Jun 8, 2023
@eladkal eladkal added the type:misc/internal Changelog: Misc changes that should appear in change log label Jun 8, 2023
eladkal pushed a commit that referenced this pull request Jun 8, 2023
By the looks of it, this param is unused.  Since the class is designated private, it is permissable to remove it.

(cherry picked from commit 0cbc0dc)
eladkal pushed a commit that referenced this pull request Jun 9, 2023
By the looks of it, this param is unused.  Since the class is designated private, it is permissable to remove it.

(cherry picked from commit 0cbc0dc)
@dstandish dstandish added the AIP-52 Automatic setup and teardown tasks label Jun 23, 2023
@jedcunningham jedcunningham added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-52 Automatic setup and teardown tasks changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) 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.

6 participants