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

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

Merged
merged 16 commits into from
Nov 14, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c1a6f89
feat: add rest transport generation for clients
yon-mg Nov 2, 2020
bd7c32d
feat: add rest transport generation for clients
yon-mg Nov 2, 2020
cd3a7f1
Merge branch 'master' of github.com:yon-mg/gapic-generator-python
yon-mg Nov 2, 2020
b41db31
feat: add transport flag
yon-mg Nov 6, 2020
ce69e7d
refactor: moved template logic outside
yon-mg Nov 6, 2020
89fa08b
fix: small fixes in transport option logic
yon-mg Nov 9, 2020
86f3a9c
test: added unit test for transport flag
yon-mg Nov 9, 2020
234cc09
test: add unit test for http option method
yon-mg Nov 11, 2020
35ac276
test: add unit test for http option method branch
yon-mg Nov 11, 2020
c8155a5
fix: fix import paths
yon-mg Nov 11, 2020
55a815c
fix: style check issues
yon-mg Nov 11, 2020
ac0bdef
fix: more style check issues
yon-mg Nov 11, 2020
e79e8c7
fix: addressing pr reviews
yon-mg Nov 13, 2020
824ad11
fix: typo in test_method
yon-mg Nov 13, 2020
2d11e19
fix: style check fixes
yon-mg Nov 13, 2020
b5c6d06
Merge branch 'master' into master
yon-mg Nov 14, 2020
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
5 changes: 1 addition & 4 deletions gapic/generator/generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,10 +299,7 @@ def _render_template(
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
return any(transport in template_name for transport in desired_transports)

def _get_file(
self,
Expand Down
7 changes: 3 additions & 4 deletions gapic/schema/wrappers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,15 +739,14 @@ def field_headers(self) -> Sequence[str]:
return next((tuple(pattern.findall(verb)) for verb in potential_verbs if verb), ())

@property
def http_opt(self) -> Dict[str, str]:
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()
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
if len(http) < 1:
return answer
return None

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 = {
answer: Dict[str, str] = {
'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.

}
Expand Down
31 changes: 31 additions & 0 deletions tests/unit/generator/test_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,37 @@ def test_get_response_fails_invalid_file_paths():
ex_str = str(ex.value)
assert "%proto" in ex_str and "%service" in ex_str

def test_get_response_ignores_unwanted_transports():
g = make_generator()
with mock.patch.object(jinja2.FileSystemLoader, "list_templates") as lt:
lt.return_value = [
"foo/%service/transports/river.py.j2",
"foo/%service/transports/car.py.j2",
"foo/%service/transports/grpc.py.j2",
"foo/%service/transports/__init__.py.j2",
"foo/%service/transports/base.py.j2",
"mollusks/squid/sample.py.j2",
]
with mock.patch.object(jinja2.Environment, "get_template") as gt:
gt.return_value = jinja2.Template("Service: {{ service.name }}")
cgr = g.get_response(api_schema=make_api(
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting nit: putting this on a new line makes it slightly easier to read the entire thing.

make_proto(
descriptor_pb2.FileDescriptorProto(
service=[
descriptor_pb2.ServiceDescriptorProto(
name="SomeService"),
]
),
)
),
opts=Options.build("transport=river+car"))
assert len(cgr.file) == 4
assert {i.name for i in cgr.file} == {
"foo/some_service/transports/river.py",
"foo/some_service/transports/car.py",
"foo/some_service/transports/__init__.py",
"foo/some_service/transports/base.py",
}

def test_get_response_enumerates_services():
g = make_generator()
Expand Down