Clarify variable names and use cleaner set diff syntax#20657
Conversation
The word 'old' could mean 'triggers we already have' i.e. 'current triggers' as compared with the 'requested' triggers, or with 'triggers we need to create' a.k.a. new triggers. However, here its usage is actually 'triggers we need to remove'. So, updating the variable to use the word 'remove' instead of 'old' is clearer. Additionally I take the opportunity to use the `-` operator in place of calling method `difference` -- I think this results in much better readability.
boring-cyborg
Bot
added
the
area:Scheduler
|
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
Bot
added
the
full tests needed
| 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 |
There was a problem hiding this comment.
| remove_trigger_ids = current_trigger_ids - requested_trigger_ids | |
| trigger_ids_to_remove = current_trigger_ids - requested_trigger_ids |
nit -- no strong preference
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
yeah i do agree with the suggestion... but this superceded by other PRs and i will close
thank you for helping to review
4 participants
The word 'old' could suggest 'triggers we already have' i.e. 'current triggers' as compared with the 'requested' triggers, or with 'triggers we need to create' a.k.a. new triggers. However, here its usage is actually 'triggers we need to remove'. So, updating the variable to use the word 'remove' instead of 'old' is clearer.
Additionally I take the opportunity to use the
-operator in place of calling methoddifference-- I think this results in much better readability.