You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/01/04 18:48:14 UTC
[GitHub] [airflow] dstandish opened a new pull request #20657: Clarify variable names and use cleaner set diff syntax
dstandish opened a new pull request #20657:
URL: https://github.com/apache/airflow/pull/20657
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 method `difference` -- I think this results in much better readability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20657: Clarify variable names and use cleaner set diff syntax
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20657:
URL: https://github.com/apache/airflow/pull/20657#discussion_r779293052
##########
File path: airflow/jobs/triggerer_job.py
##########
@@ -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
Review 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.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #20657: Clarify variable names and use cleaner set diff syntax
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #20657:
URL: https://github.com/apache/airflow/pull/20657#issuecomment-1006983265
superceded by https://github.com/apache/airflow/pull/20658 and https://github.com/apache/airflow/pull/20699
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20657: Clarify variable names and use cleaner set diff syntax
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20657:
URL: https://github.com/apache/airflow/pull/20657#discussion_r779293052
##########
File path: airflow/jobs/triggerer_job.py
##########
@@ -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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish closed pull request #20657: Clarify variable names and use cleaner set diff syntax
Posted by GitBox <gi...@apache.org>.
dstandish closed pull request #20657:
URL: https://github.com/apache/airflow/pull/20657
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #20657: Clarify variable names and use cleaner set diff syntax
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20657:
URL: https://github.com/apache/airflow/pull/20657#issuecomment-1005392592
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #20657: Clarify variable names and use cleaner set diff syntax
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20657:
URL: https://github.com/apache/airflow/pull/20657#discussion_r778662088
##########
File path: airflow/jobs/triggerer_job.py
##########
@@ -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
Review comment:
```suggestion
trigger_ids_to_remove = current_trigger_ids - requested_trigger_ids
```
nit -- no strong preference
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #20657: Clarify variable names and use cleaner set diff syntax
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20657:
URL: https://github.com/apache/airflow/pull/20657#discussion_r779909710
##########
File path: airflow/jobs/triggerer_job.py
##########
@@ -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
Review comment:
yeah i do agree with the suggestion... but this superceded by other PRs and i will close
thank you for helping to review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org