Skip to content

Remove unnecessary call to keys() method on dictionaries - #34260

Merged
uranusjr merged 3 commits into
apache:mainfrom
astronomer:remove-unnecessary-keys-call
Sep 14, 2023
Merged

Remove unnecessary call to keys() method on dictionaries#34260
uranusjr merged 3 commits into
apache:mainfrom
astronomer:remove-unnecessary-keys-call

Conversation

@pankajkoti

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-cyborg boring-cyborg Bot added area:providers provider:google Google (including GCP) related issues labels Sep 10, 2023
@pankajkoti
pankajkoti force-pushed the remove-unnecessary-keys-call branch from dd8dcb1 to 0d29e89 Compare September 10, 2023 19:45
]
)
for field_name in body_to_validate.keys():
for field_name in body_to_validate:

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.

Everything fine in refactoring itself, for me a bit confusing original test which was written years ago

def test_validate_should_fail_if_body_is_none(self):
specification = []
body = None
validator = GcpBodyFieldValidator(specification, "v1")
with pytest.raises(AttributeError):
validator.validate(body)

I not familiar with such body in GCP, but I guess if we expect that only dict is valid here then we need to fix a bit of logic from raise TypeError in case if got not a dict, because it also might confuse end users if they got Attribute error

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.

I understand that it fails because None is not iterable, but the question is how None.keys() didn't fail before 🤔

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.

>>> x = None
>>> x.keys()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'NoneType' object has no attribute 'keys'
>>>

@potiuk potiuk Sep 12, 2023

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.

I think you will never get None in reality - this was really a defensive test. And AttributeError was just side effect of using keys() on None.

I think the right solution is:

Add this:

if body_to_validate is None:
   raise RuntimeError("The body passed here should never be `None`) 

and expect the RuntimeError in this test.

:param body_to_validate: body that must follow the specification
:return: None
"""
if body_to_validate is None:

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.

The function’s type annotation should probably be updated to cover the None case.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am slightly confused if it would make sense to update the type annotation to include None as an acceptable type only to raise an error. 🤔

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.

Yeah that’s a good point

@uranusjr
uranusjr merged commit b042042 into apache:main Sep 14, 2023
@uranusjr
uranusjr deleted the remove-unnecessary-keys-call branch September 14, 2023 03:36
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.2 milestone Oct 3, 2023
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:google Google (including GCP) related issues 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.

7 participants