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

fix: fix mypy errors in rest.py #1599

Merged
merged 9 commits into from
Feb 17, 2023
Merged

fix: fix mypy errors in rest.py #1599

merged 9 commits into from
Feb 17, 2023

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Feb 16, 2023

Fixes #1600

Also addresses mypy error in golden file credentials/google/iam/credentials_v1/types/iamcredentials.py

nox > mypy --explicit-package-bases google
google/iam/credentials_v1/types/iamcredentials.py:18: error: Name "proto" is not defined  [name-defined]

https://github.com/googleapis/gapic-generator-python/blob/main/tests/integration/goldens/credentials/google/iam/credentials_v1/types/iamcredentials.py#L18

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Feb 16, 2023
@parthea parthea changed the title fix: run mypy on golden files fix: fix mypy errors in rest.py Feb 16, 2023
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Feb 16, 2023
@parthea parthea marked this pull request as ready for review February 16, 2023 18:40
@parthea parthea requested review from a team as code owners February 16, 2023 18:40
@@ -269,7 +269,7 @@ class {{service.name}}RestTransport({{service.name}}Transport):

{% if not method.client_streaming %}
{% if method.input.required_fields %}
__REQUIRED_FIELDS_DEFAULT_VALUES: Dict[str, str] = {
__REQUIRED_FIELDS_DEFAULT_VALUES: Dict[str, Any] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the whole purpose of typing is to provide type hints, then isn't the use of Any defeating the purpose? Is there a more narrow type hint we could provide? (I see that the error message in [1] expects as the map value the type "Dict[<nothing>, <nothing>]")

[1] https://github.com/googleapis/python-bigtable/actions/runs/4196460916/jobs/7277462654

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed Any . PTAL

@@ -269,7 +269,7 @@ class {{service.name}}RestTransport({{service.name}}Transport):

{% if not method.client_streaming %}
{% if method.input.required_fields %}
__REQUIRED_FIELDS_DEFAULT_VALUES: Dict[str, str] = {
__REQUIRED_FIELDS_DEFAULT_VALUES: Dict = {
Copy link
Contributor

@vchudnov-g vchudnov-g Feb 17, 2023

Choose a reason for hiding this comment

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

But only checking for Dict does even less type checking than Dict[str, Any], right, so what's the point? I was wondering whether we could have the str check on the key but also make the type check tighter than just an Any. Does that make sense?

The more I think about it, though, the more I wonder whether it's a sensible question. I guess each required field can be of any proto-derived type, and there may be multiple required fields, so at best we could check for the union of the types of the required fields in the request message. Hmmmm.... Maybe you're right, Dict[str,Any] is the most sensible. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thought about it some more. I suggest reverting your last commit and going back to Dict[str,Any] (sorry!) unless there's any way to make the Any more specific, which I'm thinking there isn't without a lot more work for arguably small benefit.....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

This reverts commit 2f07463.
@parthea parthea enabled auto-merge (squash) February 17, 2023 14:15
@parthea parthea merged commit 120f19e into main Feb 17, 2023
@parthea parthea deleted the fix-mypy-errors branch February 17, 2023 14:17
Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mypy test failing for rest.py in generated golden files
3 participants