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

Consolidate import and usage of itertools #33479

Merged
merged 1 commit into from
Aug 21, 2023
Merged

Conversation

eumiro
Copy link
Contributor

@eumiro eumiro commented Aug 17, 2023

No description provided.

@uranusjr
Copy link
Member

If we want to consolidate (not strictly necessary) I’d prefer the full itertools name. it is used in some places as a variable (short for iterator).

airflow/www/views.py Outdated Show resolved Hide resolved
@eumiro eumiro force-pushed the itertools branch 3 times, most recently from b928305 to c94feea Compare August 19, 2023 17:40
@@ -142,7 +142,7 @@ def wrapper(self, context, *args, **kwargs):
_inlets = self.xcom_pull(
context, task_ids=task_ids, dag_id=self.dag_id, key=PIPELINE_OUTLETS, session=session
)
self.inlets.extend(i for i in itertools.chain.from_iterable(_inlets))
self.inlets.extend(itertools.chain.from_iterable(_inlets))
Copy link
Member

Choose a reason for hiding this comment

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

This can just be

Suggested change
self.inlets.extend(itertools.chain.from_iterable(_inlets))
self.inlets.extend(i for it in _inlets for i in it)

but itertools has readability advantages.

for argument, default in itertools.zip_longest(arguments, defaults, fillvalue=None):
for argument, default in zip(arguments, defaults):
Copy link
Member

Choose a reason for hiding this comment

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

These are not equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the subsequent line:

            if argument is None or default is None:
                continue

all the lines that come from zip_longest but are ignored in zip are ignored anyway.

@eumiro eumiro force-pushed the itertools branch 2 times, most recently from 524bb1c to 1f89278 Compare August 20, 2023 05:25
@potiuk
Copy link
Member

potiuk commented Aug 20, 2023

Conflicts :(

@potiuk potiuk merged commit 95a930b into apache:main Aug 21, 2023
@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:CLI area:dev-tools area:lineage area:providers area:Scheduler including HA (high availability) scheduler area:webserver Webserver related Issues kind:documentation provider:amazon-aws AWS/Amazon - related issues provider:apache-hive 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.

5 participants