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

feat: add rest transport generation for clients with optional flag #688

Merged
merged 16 commits into from
Nov 14, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions gapic/generator/generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,8 @@ def _render_template(
skip_subpackages
and service.meta.address.subpackage != api_schema.subpackage_view
or
'grpc' in template_name and 'grpc' not in opts.transport
or
'rest' in template_name and 'rest' not in opts.transport
'transport' in template_name
and not self._is_desired_transport(template_name, opts)
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Expand All @@ -297,6 +296,14 @@ 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
for transport in desired_transports:
if transport in template_name:
return True
return False
software-dov marked this conversation as resolved.
Show resolved Hide resolved

def _get_file(
self,
template_name: str,
Expand Down
9 changes: 5 additions & 4 deletions gapic/schema/wrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -742,13 +742,14 @@ def field_headers(self) -> Sequence[str]:
def http_opt(self) -> 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()
Copy link
Contributor

Choose a reason for hiding this comment

The 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 List[Tuple[FieldDescriptor, str]] would be helpful.

answer: Dict[str, str] = None
software-dov marked this conversation as resolved.
Show resolved Hide resolved
if len(http) < 1:
return None
return answer

http_method = http[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on our offline discussion, I suggest s/http_method/primary_binding/ for clarity.

answer: Dict[str, str] = {
'method':http_method[0].name,
'url':http_method[1],
answer = {
'method': http_method[0].name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on our offline discussion, let's s/'method'/'verb'/, since "method" is overloaded (service method, object method, HTTP method)

Copy link
Contributor

Choose a reason for hiding this comment

The 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],
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider having "template" in the name: maybe 'path_template' would be more descriptive and accurate.

}
if len(http) > 1:
http_opt = http[1]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,9 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):
# Return the client from cache.
return self._operations_client
{%- endif %}


{%- for method in service.methods.values() %}
{%- if method.http_opt %}

def {{ method.name|snake_case }}(self,
request: {{ method.input.ident }}, *,
metadata: Sequence[Tuple[str, str]] = (),
Expand Down Expand Up @@ -162,11 +161,10 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):
{%- endif %}
)
{%- if not method.void %}
{%- endif %}

# Return the response
return {{ method.output.ident }}.from_json(response.content)
{%- else %}
{%- endif %}
{%- endif %}
{%- endfor %}

Expand Down
5 changes: 3 additions & 2 deletions gapic/utils/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ class Options:
lazy_import: bool = False
old_naming: bool = False
add_iam_methods: bool = False
# TODO(yonmg): should there be an enum for transport type?
transport: List[str] = None
# TODO(yon-mg): should there be an enum for transport type?
transport: List[str] = dataclasses.field(default_factory=lambda: [])

# Class constants
PYTHON_GAPIC_PREFIX: str = 'python-gapic-'
Expand Down Expand Up @@ -138,6 +138,7 @@ def tweak_path(p):
lazy_import=bool(opts.pop('lazy-import', False)),
old_naming=bool(opts.pop('old-naming', False)),
add_iam_methods=bool(opts.pop('add-iam-methods', False)),
# transport should include desired transports delimited by '+', e.g. transport='grpc+rest'
transport=opts.pop('transport', ['grpc'])[0].split('+')
software-dov marked this conversation as resolved.
Show resolved Hide resolved
)

Expand Down