Skip to content
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

Simplify 'X for X in Y' to 'Y' where applicable #33453

Merged
merged 1 commit into from
Aug 20, 2023

Conversation

eumiro
Copy link
Contributor

@eumiro eumiro commented Aug 16, 2023

Simplify loops without condition X for X in Y to bare Y, or convert it to list() or set() or iter() if needed.

@@ -134,7 +134,7 @@ def pytest_print(text):
# It is very unlikely that the user wants to display only numbers, but probably
# the user just wants to count the queries.
exit_stack.enter_context(count_queries(print_fn=pytest_print))
elif any(c for c in ["time", "trace", "sql", "parameters"]):
elif any(c in columns for c in ["time", "trace", "sql", "parameters"]):
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch

@@ -557,7 +557,7 @@ def test__write_local_data_files_csv_does_not_write_on_empty_rows(self):
with pytest.raises(StopIteration):
next(files)["file_handle"]

assert len([f for f in files]) == 0
assert not list(files)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert not list(files)
with pytest.raises(StopIteration):
next(files)

Alternative approach. Shouldn’t be too much difference in practice (especially in a test).

Copy link
Contributor Author

@eumiro eumiro Aug 18, 2023

Choose a reason for hiding this comment

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

This is really elegant. Anyway, since the previous block already expects a StopIteration, we do not have to repeat it here. I'll remove it altogether.

@potiuk potiuk merged commit 7700fb1 into apache:main Aug 20, 2023
@eumiro eumiro deleted the simplify-loops branch August 20, 2023 18:19
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Aug 27, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.1 milestone Aug 27, 2023
ephraimbuddy pushed a commit that referenced this pull request Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow area:dev-tools area:lineage area:logging area:production-image Production image improvements and fixes area:providers area:system-tests area:webserver Webserver related Issues provider:amazon AWS/Amazon - related issues provider:apache-hive provider:google Google (including GCP) related issues provider:microsoft-azure Azure-related issues provider:smtp type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants