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: use correct typing for retries #1026

Merged
merged 6 commits into from
Oct 13, 2021

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Oct 13, 2021

fix: import 'ClientOptions' class correctly

Closes #1024
Closes #1025

@tseaver tseaver requested a review from a team as a code owner October 13, 2021 21:48
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 13, 2021
@tseaver tseaver requested a review from crwilcox October 13, 2021 21:48
@@ -22,6 +22,8 @@ from google.auth.transport.grpc import SslCredentials # type: ignore
from google.auth.exceptions import MutualTLSChannelError # type: ignore
from google.oauth2 import service_account # type: ignore

OptionalRetry = Union[retries.Retry, object]
Copy link

Choose a reason for hiding this comment

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

Wouldn't this be equivalent to Any type?

Copy link

Choose a reason for hiding this comment

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

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'm not sure. I do know that gapic_v1.method.DEFAULT_RETRY is just an instance of object.

Copy link

Choose a reason for hiding this comment

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

Right. That's what I mean. object is a very generic type. Basically everything is object, so union with that is basically Any.

Would folks be open to making a more explicit type for the default sentinel like we did in BQ?

Copy link

Choose a reason for hiding this comment

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

To test if it is equivalent to Any, we could try passing a string as a retry argument. It ideally should fail type checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe DEFAULT_RETRY is a specific object used as a canary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we update api-core to provide a different type for the marker object, then we can tweak the templates to use it -- otherwise, the typechecker hates that we are using that specific object as the default when we say the parameter is a retries.Retry.

Copy link

Choose a reason for hiding this comment

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

Okay. Well I suspect this makes the type checker happy with any object, which isn't what we want. Did you see the pattern I used in google-cloud-bigquery? That pattern uses the fact that enums can be type checked to look for a specific object.

@tseaver tseaver force-pushed the 1024-1025-fix-retry-client_options-types branch from f604149 to 4ae06f1 Compare October 13, 2021 22:08
@tseaver tseaver merged commit acb3ea8 into master Oct 13, 2021
@tseaver tseaver deleted the 1024-1025-fix-retry-client_options-types branch October 13, 2021 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async clients mis-import 'ClientOptions' class mypy reports typing for retry arguments is invalid
3 participants