Skip to content

Conversation

@Taragolis
Copy link
Contributor

A minor change logic in conditions with empty block in it

@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Nov 18, 2022
Comment on lines 540 to 543
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Member

@potiuk potiuk Nov 21, 2022

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).

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper would be useful in another places

image

@eladkal eladkal requested a review from XD-DENG November 24, 2022 07:25
@potiuk potiuk dismissed XD-DENG’s stale review December 3, 2022 20:44

The comments were addressed :)

@potiuk potiuk merged commit 4a3a429 into apache:main Dec 3, 2022
@Taragolis Taragolis deleted the simplify-conditions branch January 14, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants