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.

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

Jump to bottom

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

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

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion google/api_core/gapic_v1/method.py
View file
Open in desktop
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

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

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

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

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

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

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