potiuk commented on code in PR #27775:
URL: https://github.com/apache/airflow/pull/27775#discussion_r1028460927
##########
airflow/providers/google/cloud/hooks/pubsub.py:
##########
@@ -537,12 +537,10 @@ def acknowledge(
individual attempt.
:param metadata: (Optional) Additional metadata that is provided to
the method.
"""
- if ack_ids is not None and messages is None:
- pass
- elif ack_ids is None and messages is not None:
- ack_ids = [message.ack_id for message in messages]
- else:
+ if not (ack_ids is None) ^ (messages is None):
raise ValueError("One and only one of 'ack_ids' and 'messages'
arguments have to be provided")
+ elif ack_ids is None:
+ ack_ids = [message.ack_id for message in messages] # type:
ignore[union-attr]
Review Comment:
Agree. The first one is more readable. I think it's not necesary True that
"shorter == more readable" . I for example prefer to write classic nested for
loop instead of nested for comprehension because I find it much better
expressing the intention even if the latter is one-liner.
But in this case I think we have nice util that might make it even more
readable - we have `airflow.utils.helpers.exactly_one` (added long time enough
to pass >=2.3) and we could use that one making it most readable (even if not
that efficient but in this case micro-optimisaatattion might be premature.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
