-
Notifications
You must be signed in to change notification settings - Fork 69
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
feat: add rest transport generation for clients with optional flag #688
Changes from 7 commits
c1a6f89
bd7c32d
cd3a7f1
b41db31
ce69e7d
89fa08b
86f3a9c
234cc09
35ac276
c8155a5
55a815c
ac0bdef
e79e8c7
824ad11
2d11e19
b5c6d06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,7 +42,6 @@ def generate( | |
|
||
# Pull apart arguments in the request. | ||
opts = Options.build(req.parameter) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I like having the blank line here for readability. |
||
# Determine the appropriate package. | ||
# This generator uses a slightly different mechanism for determining | ||
# which files to generate; it tracks at package level rather than file | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -274,6 +274,9 @@ def _render_template( | |
if ( | ||
skip_subpackages | ||
and service.meta.address.subpackage != api_schema.subpackage_view | ||
or | ||
'transport' in template_name | ||
and not self._is_desired_transport(template_name, opts) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you put parentheses around this chunk, just to make it clear that it's a cohesive unit? |
||
): | ||
continue | ||
|
||
|
@@ -293,6 +296,11 @@ def _render_template( | |
template_name, api_schema=api_schema, opts=opts)) | ||
return answer | ||
|
||
def _is_desired_transport(self, template_name: str, opts: Options) -> bool: | ||
"""Returns true if template name contains a desired transport""" | ||
desired_transports = ['__init__', 'base'] + opts.transport | ||
return any(transport in template_name for transport in desired_transports) | ||
|
||
def _get_file( | ||
self, | ||
template_name: str, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -719,6 +719,7 @@ def _client_output(self, enable_asyncio: bool): | |
# Return the usual output. | ||
return self.output | ||
|
||
# TODO(yonmg): remove or rewrite. don't think it performs as intended | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Suggest adding just a tiny bit more detail. It often happens that the person working on the TODO is not the person who wrote it, so adding some context is a helpful practice. |
||
@property | ||
def field_headers(self) -> Sequence[str]: | ||
"""Return the field headers defined for this method.""" | ||
|
@@ -736,7 +737,28 @@ def field_headers(self) -> Sequence[str]: | |
] | ||
|
||
return next((tuple(pattern.findall(verb)) for verb in potential_verbs if verb), ()) | ||
|
||
@property | ||
def http_opt(self) -> Optional[Dict[str, str]]: | ||
"""Return the http option for this method.""" | ||
yon-mg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
http = self.options.Extensions[annotations_pb2.http].ListFields() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think a type declaration or a comment saying this is a |
||
if len(http) < 1: | ||
return None | ||
|
||
http_method = http[0] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on our offline discussion, I suggest s/ |
||
answer: Dict[str, str] = { | ||
'method': http_method[0].name, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on our offline discussion, let's s/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this be madness, there be method to it! 💀 |
||
'url': http_method[1], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider having "template" in the name: maybe |
||
} | ||
if len(http) > 1: | ||
http_opt = http[1] | ||
answer[http_opt[0].name] = http_opt[1] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For clarity I would suggest
|
||
|
||
# TODO(yonmg): handle nested fields & fields past body i.e. 'additional bindings' | ||
# TODO(yonmg): enums for http verbs? | ||
return answer | ||
|
||
# TODO(yonmg): refactor as there may be more than one method signature | ||
@utils.cached_property | ||
def flattened_fields(self) -> Mapping[str, Field]: | ||
"""Return the signature defined for this method.""" | ||
|
@@ -786,6 +808,7 @@ def grpc_stub_type(self) -> str: | |
server='stream' if self.server_streaming else 'unary', | ||
) | ||
|
||
# TODO(yonmg): figure out why idempotent is reliant on http annotation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. http GET requests are automatically considered idempotent because they do not change data on the server. Post requests are usually non-idempotent, because it is usually unsafe retry write operation if it failed, because it may lead for example into writing the same information twice on the server. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get that GET is idempotent but couldn't there be methods without an http annotation that are also idempotent? are grpc only methods inherently not so? |
||
@utils.cached_property | ||
def idempotent(self) -> bool: | ||
"""Return True if we know this method is idempotent, False otherwise. | ||
|
@@ -980,6 +1003,10 @@ def grpc_transport_name(self): | |
def grpc_asyncio_transport_name(self): | ||
return self.name + "GrpcAsyncIOTransport" | ||
|
||
@property | ||
def rest_transport_name(self): | ||
return self.name + "RestTransport" | ||
|
||
@property | ||
def has_lro(self) -> bool: | ||
"""Return whether the service has a long-running method.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -151,6 +151,7 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport): | |
) | ||
|
||
self._stubs = {} # type: Dict[str, Callable] | ||
{%- if service.has_lro %} self._operations_client = None {%- endif %} | ||
|
||
# Run the base constructor. | ||
super().__init__( | ||
|
@@ -172,7 +173,7 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport): | |
**kwargs) -> grpc.Channel: | ||
"""Create and return a gRPC channel object. | ||
Args: | ||
address (Optionsl[str]): The host for the channel to use. | ||
software-dov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
address (Optional[str]): The host for the channel to use. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this changed (looks unrelated to your changes)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I approve of fixing typos on the fly! |
||
credentials (Optional[~.Credentials]): The | ||
authorization credentials to attach to requests. These | ||
credentials identify this application to the service. If | ||
|
@@ -220,13 +221,13 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport): | |
client. | ||
""" | ||
# Sanity check: Only create a new client if we do not already have one. | ||
if 'operations_client' not in self.__dict__: | ||
self.__dict__['operations_client'] = operations_v1.OperationsClient( | ||
if self._operations_client is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. won't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a little bit complicated. Some values can be falsy even if they are non-null, e.g. empty lists and strings. |
||
self._operations_client = operations_v1.OperationsClient( | ||
self.grpc_channel | ||
) | ||
|
||
# Return the client from cache. | ||
return self.__dict__['operations_client'] | ||
return self._operations_client | ||
{%- endif %} | ||
{%- for method in service.methods.values() %} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,6 +205,7 @@ class {{ service.grpc_asyncio_transport_name }}({{ service.name }}Transport): | |
) | ||
|
||
self._stubs = {} | ||
{%- if service.has_lro %} self._operations_client = None {%- endif %} | ||
|
||
@property | ||
def grpc_channel(self) -> aio.Channel: | ||
|
@@ -225,13 +226,13 @@ class {{ service.grpc_asyncio_transport_name }}({{ service.name }}Transport): | |
client. | ||
""" | ||
# Sanity check: Only create a new client if we do not already have one. | ||
if 'operations_client' not in self.__dict__: | ||
self.__dict__['operations_client'] = operations_v1.OperationsAsyncClient( | ||
if self._operations_client is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you changing this code (looks unrelated to http transport stuff)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #688 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for consistency |
||
self._operations_client = operations_v1.OperationsAsyncClient( | ||
self.grpc_channel | ||
) | ||
|
||
# Return the client from cache. | ||
return self.__dict__['operations_client'] | ||
return self._operations_client | ||
{%- endif %} | ||
{%- for method in service.methods.values() %} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use your github id for TODOs (here and in the other places)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1