Skip to content

replace unnecessary dict comprehension by dict() in core - #33858

Merged
uranusjr merged 4 commits into
apache:mainfrom
hussein-awala:unacessary_dict_core
Sep 11, 2023
Merged

replace unnecessary dict comprehension by dict() in core#33858
uranusjr merged 4 commits into
apache:mainfrom
hussein-awala:unacessary_dict_core

Conversation

@hussein-awala

Copy link
Copy Markdown
Member

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added area:API Airflow's REST/HTTP API area:core-operators area:webserver Webserver related Issues labels Aug 28, 2023
@hussein-awalahussein-awala changed the title replace unacessary dict comprehension by dict() in corereplace unnecessary dict comprehension by dict() in coreAug 28, 2023
@hussein-awala
hussein-awala marked this pull request as draft August 28, 2023 21:42
Comment threadairflow/api/common/experimental/get_lineage.py Outdated
Comment threadairflow/operators/python.py Outdated

def _execute_python_callable_in_subprocess(self, python_path: Path, tmp_dir: Path):
op_kwargs: dict[str, Any] = {k: v for k, v in self.op_kwargs.items()}
op_kwargs: dict[str, Any] = dict(self.op_kwargs.items())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
op_kwargs: dict[str, Any] =dict(self.op_kwargs.items())
op_kwargs: dict[str, Any] =self.op_kwargs.copy()

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

op_kwargs type is Mapping that doesn't have copy() method

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then dict(self.op_kwargs) should work, I think.

Comment threadairflow/www/decorators.py Outdated
Comment threadairflow/www/views.py
Comment threadairflow/models/dagrun.py Outdated
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
Comment threadairflow/www/views.py
@hussein-awala
hussein-awala marked this pull request as ready for review August 30, 2023 21:40
lineage[meta.task_id]["outlets"] = meta.value

return {"task_ids": {k: v for k, v in lineage.items()}}
return {"task_ids": lineage.copy()}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return {"task_ids": lineage.copy()}
return {"task_ids": dict(lineage)}

Otherwise you've got a copy of defaultdict rather than dict

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don’t think that’d be problematic though?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree with @Taragolis, I should avoid any change in the values in this refactor PR

@uranusjr
uranusjr merged commit 6a33d05 into apache:mainSep 11, 2023
@ephraimbuddyephraimbuddy added this to the Airflow 2.7.2 milestone Oct 3, 2023
@ephraimbuddyephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 3, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 5, 2023
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
(cherry picked from commit 6a33d05)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:APIAirflow's REST/HTTP APIarea:webserverWebserver related Issuestype:misc/internalChangelog: Misc changes that should appear in change log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@hussein-awala@uranusjr@Taragolis@ephraimbuddy