Skip to content

Clarify variable names and use cleaner set diff syntax #20657

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.

Sign up for GitHub

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

Closed
dstandish wants to merge 1 commit into from
Closed

Clarify variable names and use cleaner set diff syntax #20657

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions airflow/jobs/triggerer_job.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,8 +382,8 @@ def update_triggers(self, requested_trigger_ids: Set[int]):
# handling.
current_trigger_ids = set(self.triggers.keys())
# Work out the two difference sets
new_trigger_ids = requested_trigger_ids.difference(current_trigger_ids)
old_trigger_ids = current_trigger_ids.difference(requested_trigger_ids)
new_trigger_ids = requested_trigger_ids - current_trigger_ids
remove_trigger_ids = current_trigger_ids - requested_trigger_ids

@kaxil kaxil Jan 5, 2022

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.

Suggested change
remove_trigger_ids = current_trigger_ids - requested_trigger_ids
trigger_ids_to_remove = current_trigger_ids - requested_trigger_ids

nit -- no strong preference

@uranusjr uranusjr Jan 6, 2022 •
edited
Loading

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.

@dstandish Do you want to change this? Personally I like the ...to_remove name slightly better as well since remove_... sounds like a verb, not a noun. But if you like the current name better, let’s get this merged as-is.

@dstandish dstandish Jan 6, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah i do agree with the suggestion... but this superceded by other PRs and i will close

thank you for helping to review

# Bulk-fetch new trigger records
new_triggers = Trigger.bulk_fetch(new_trigger_ids)
# Add in new triggers
Expand All @@ -400,9 +400,9 @@ def update_triggers(self, requested_trigger_ids: Set[int]):
self.failed_triggers.append(new_id)
continue
self.to_create.append((new_id, trigger_class(**new_triggers[new_id].kwargs)))
# Remove old triggers
for old_id in old_trigger_ids:
self.to_delete.append(old_id)
# Remove unneeded triggers
for remove_id in remove_trigger_ids:
self.to_delete.append(remove_id)

def get_trigger_by_classpath(self, classpath: str) -> Type[BaseTrigger]:
"""
Expand Down