-
Notifications
You must be signed in to change notification settings - Fork 16.4k
[misc] Get rid of pass statement in conditions
#27775
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
Conversation
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