You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "pankajkoti (via GitHub)" <gi...@apache.org> on 2023/09/10 19:44:56 UTC
[GitHub] [airflow] pankajkoti opened a new pull request, #34260: Remove unnecessary call to keys() method on dictionaries
pankajkoti opened a new pull request, #34260:
URL: https://github.com/apache/airflow/pull/34260
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34260:
URL: https://github.com/apache/airflow/pull/34260#discussion_r1324243936
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
+ if body_to_validate is None:
+ raise RuntimeError("The body to validate is `None`. Please provide a dictionary to validate.")
Review Comment:
Why is this needed? The argument is typed as `dict`, is it used incorrectly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #34260:
URL: https://github.com/apache/airflow/pull/34260#discussion_r1322423506
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -443,7 +443,7 @@ def validate(self, body_to_validate: dict) -> None:
and nested_union_spec.get("api_version") != self._api_version
]
)
- for field_name in body_to_validate.keys():
+ for field_name in body_to_validate:
Review Comment:
```
>>> x = None
>>> x.keys()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'NoneType' object has no attribute 'keys'
>>>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #34260:
URL: https://github.com/apache/airflow/pull/34260#discussion_r1322114007
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -443,7 +443,7 @@ def validate(self, body_to_validate: dict) -> None:
and nested_union_spec.get("api_version") != self._api_version
]
)
- for field_name in body_to_validate.keys():
+ for field_name in body_to_validate:
Review Comment:
I understand that it fails because None is not iterable, but the question is how None.keys() didn't fail before :thinking:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #34260:
URL: https://github.com/apache/airflow/pull/34260#discussion_r1320867479
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -443,7 +443,7 @@ def validate(self, body_to_validate: dict) -> None:
and nested_union_spec.get("api_version") != self._api_version
]
)
- for field_name in body_to_validate.keys():
+ for field_name in body_to_validate:
Review Comment:
Everything fine in refactoring itself, for me a bit confusing original test which was written years ago
https://github.com/apache/airflow/blob/dd4804e05bb7e994a3782feacf47c89d2e9481f3/tests/providers/google/cloud/utils/test_field_validator.py#L37-L44
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34260:
URL: https://github.com/apache/airflow/pull/34260#discussion_r1324244863
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
+ if body_to_validate is None:
Review Comment:
The function’s type annotation should probably be updated to cover the None case.
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
+ if body_to_validate is None:
Review Comment:
The function’s type annotation should probably be updated to cover the None case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pankajkoti commented on a diff in pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #34260:
URL: https://github.com/apache/airflow/pull/34260#discussion_r1324257695
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
+ if body_to_validate is None:
Review Comment:
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. 🤔
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
+ if body_to_validate is None:
Review Comment:
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. 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34260:
URL: https://github.com/apache/airflow/pull/34260#discussion_r1324471018
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
+ if body_to_validate is None:
Review Comment:
Yeah that’s a good point
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
+ if body_to_validate is None:
Review Comment:
Yeah that’s a good point
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #34260:
URL: https://github.com/apache/airflow/pull/34260#discussion_r1322444484
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -443,7 +443,7 @@ def validate(self, body_to_validate: dict) -> None:
and nested_union_spec.get("api_version") != self._api_version
]
)
- for field_name in body_to_validate.keys():
+ for field_name in body_to_validate:
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34260:
URL: https://github.com/apache/airflow/pull/34260#discussion_r1324243936
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
+ if body_to_validate is None:
+ raise RuntimeError("The body to validate is `None`. Please provide a dictionary to validate.")
Review Comment:
WHy is this needed? The argument is typed as `dict`, is it used incorrectly?
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
+ if body_to_validate is None:
+ raise RuntimeError("The body to validate is `None`. Please provide a dictionary to validate.")
Review Comment:
WHhy is this needed? The argument is typed as `dict`, is it used incorrectly?
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
+ if body_to_validate is None:
+ raise RuntimeError("The body to validate is `None`. Please provide a dictionary to validate.")
Review Comment:
Why is this needed? The argument is typed as `dict`, is it used incorrectly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr merged PR #34260:
URL: https://github.com/apache/airflow/pull/34260
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #34260: Remove unnecessary call to keys() method on dictionaries
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #34260:
URL: https://github.com/apache/airflow/pull/34260#discussion_r1322444484
##########
airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -443,7 +443,7 @@ def validate(self, body_to_validate: dict) -> None:
and nested_union_spec.get("api_version") != self._api_version
]
)
- for field_name in body_to_validate.keys():
+ for field_name in body_to_validate:
Review Comment:
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 catch the RuntimeError in this test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org