Skip to content

fix: make 'gapic_v1.method.DEFAULT' a typed object #292

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 2, 2021
Merged

fix: make 'gapic_v1.method.DEFAULT' a typed object #292

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad23dd7
fix: make 'gapic_v1.method.DEFAULT' a typed object
tseaver Oct 14, 2021
349b8fa
Merge branch 'main' into make-method-default-typable
tseaver Nov 2, 2021
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion google/api_core/gapic_v1/method.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,26 @@
pagination, and long-running operations to gRPC methods.
"""

import enum
import functools

from google.api_core import grpc_helpers
from google.api_core import timeout
from google.api_core.gapic_v1 import client_info

USE_DEFAULT_METADATA = object()
DEFAULT = object()


class _MethodDefault(enum.Enum):
Copy link
Contributor

@tswast tswast Oct 14, 2021

Choose a reason for hiding this comment

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

Since this needs to be used in the generated code, should we make this public?
Copy link
Contributor Author

@tseaver tseaver Oct 14, 2021

Choose a reason for hiding this comment

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

I figured that the generator is in the "friends zone" for api-core. :)
Copy link
Contributor

@crwilcox crwilcox Oct 14, 2021

Choose a reason for hiding this comment

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

I think the concern is that this type will be exposed as a type to users of the API?
Copy link
Contributor Author

@tseaver tseaver Oct 15, 2021

Choose a reason for hiding this comment

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

It seems like an "implementation detail" to me, uninteresting to anyone. It will be in generated code, and present only in the definition of the OptionalRetry type -- see googleapis/gapic-generator-python#1032 (review). I am in fact considering a change to that PR which removes even the classname. Instead of:

try: OptionalRetry = Union[retries.Retry, gapic_v1.method._MethodDefault] except AttributeError: OptionalRetry = Union[retries.Retry, object]

I'm thinking this would be simpler / clearer:

OptionalRetry = Union[retries.Retry, type(gapic_v1.method.DEFAULT)]
Copy link
Contributor Author

@tseaver tseaver Oct 15, 2021

Choose a reason for hiding this comment

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

Unfortunately, the latter does not work.
Copy link
Contributor

@tswast tswast Nov 1, 2021

Choose a reason for hiding this comment

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

I think the concern is that this type will be exposed as a type to users of the API?

Yes, that is a concern.

On second thought though, I think most users usages of the _MethodDefault type will be by omitting the retry argument, so they probably won't need to access this directly. Especially not if OptionalRetry is exposed in the generated client.
# Uses enum so that pytype/mypy knows that this is the only possible value.
# https://stackoverflow.com/a/60605919/101923
#
# Literal[_DEFAULT_VALUE] is an alternative, but only added in Python 3.8.
# https://docs.python.org/3/library/typing.html#typing.Literal
_DEFAULT_VALUE = object()


DEFAULT = _MethodDefault._DEFAULT_VALUE
"""Sentinel value indicating that a retry or timeout argument was unspecified,
so the default should be used."""

Expand Down