-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
[misc] Get rid of pass
statement in conditions
#27775
Conversation
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of like the old code actually, it’s a little easier to reason with, but that may be subjective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I revisit to current code and also thought you right and XOR operation actually confuse mypy
.
I would revert this changes and add comment to empty block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @uranusjr on this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return to initial variant. Just add short comments and tests.
Unfortunetly airflow.utils.helpers.exactly_one
cannot use in this case because first argument (as well as second) for check would be Iterable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f121ba1
to
9583b7d
Compare
A minor change logic in conditions with empty block in it