Initial commit for generating an integrated sample #137
Conversation
Adds basic functionality and tests for end-to-end sample generation.
Includes tests for comprehensive sample generation
Conduct translation from special variable '$resp' to 'response' in
template rendering
Includes macros to set up method invocation, adds code to include
license and copyright information,
Note: sample generation is still ignorant of method and message
signatures, types, attributes, and so forth.
More full-featured semantic analysis will be conducted
in subsequent PRs.
Includes minor tweaks to various names, tests, and macros.
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 21 21
Lines 928 975 +47
Branches 198 206 +8
=====================================
+ Hits 928 975 +47
Continue to review full report at Codecov.
|
Update type annotations for utils.code.partition Flesh out docstrings
|
There are some changes which seem to be code formatting only. Is it possible to have a separate PR dedicated to fix formatting issues? |
|
I'll open an issue for formatting tweaks and make a separate PR to address the issue. |
Replace more manually newlined strings with triple-quoted strings
| naming=naming, | ||
| prior_protos=prior_protos or {}, | ||
| ).proto | ||
| file_to_generate=file_to_generate, |
There was a problem hiding this comment.
What auto-formatter are you using? Perhaps we should settle on one and reify it in CI.
There was a problem hiding this comment.
autopep8, part of my python-mode-hook.
Opened issue #142 as a feature task for CI. Do you have strong feelings on autopep8 vs black or something else? My vote is black because there are no choices to make.
There was a problem hiding this comment.
I am not willing to use black because it generates non-PEP-8 compliant code. autopep8 is fine with me.
There was a problem hiding this comment.
Most of what I've read about black indicates that its output is PEP-8 compliant. What parts does it violate? Just curious.
Change the order of the parameters in 'partition' Remove license and copyright information from the sample template s/resp_to_response/coerce_variable_name/g Add the python package name in the pip install comment in the sample header
There was a problem hiding this comment.
Mostly minor comments, but some highlights:
- License: I suggest a placeholder so we can verify things work until we implement reading the license from a file (and have a test for that new functionality)
- Don't need the
*_coreregion tags for now (yes, that's a recent change)
I agree that making wholesale formatting changes, especially if they touch code not already under review, is better left to a subsequent PR.
gapic/samplegen/samplegen.py
Outdated
| @@ -92,6 +103,20 @@ class InvalidRequestSetup(SampleError): | |||
| pass | |||
|
|
|||
|
|
|||
| def coerce_variable_name(s): | |||
There was a problem hiding this comment.
If the intent of this is to remain specific to replacing $resp (ie, it only takes one argument), I suggest renaming to coerce_response_name
Also, is the intent for the signature above and logic below to grow to accommodate the extra cases where the target is not "response"? (eg for paged methods, etc.)
There was a problem hiding this comment.
I'm not sure I understand your question. The ideal solution to the problem this functionlet/filter is trying to solve would be
sed -i "s/\$resp/response/g" sample_file.py
Handling it in the validation code is fiddly, actually using sed in the pipeline won't work and doing a big string replace in python code that invokes rendering feels like a hack, so the substitution is punted to the template.
There was a problem hiding this comment.
The question was referring to the fact that sometimes we may choose to have $resp have a slightly different name, eg here and here
I think it's fine for each µgen to decide independently whether or not to always $resp→response in these cases or use an alternate name, though the set of alternate names should be small across languages to minimize forbidden user-provided identifiers.
There was a problem hiding this comment.
The consistency that I aimed for in the calling-form handling template code is that response is always the most interesting top level proto response, i.e. it's the repeated element in paginated or streamed return values, it's the result of waiting on a promise, and so forth.
Integrate minor review comments: Remove _core comment in rendered output s/coerce_variable_name/coerce_response_name Add back the license and copyright headers with dummy values
gapic/samplegen/samplegen.py
Outdated
| @@ -92,6 +103,20 @@ class InvalidRequestSetup(SampleError): | |||
| pass | |||
|
|
|||
|
|
|||
| def coerce_variable_name(s): | |||
There was a problem hiding this comment.
The question was referring to the fact that sometimes we may choose to have $resp have a slightly different name, eg here and here
I think it's fine for each µgen to decide independently whether or not to always $resp→response in these cases or use an alternate name, though the set of alternate names should be small across languages to minimize forbidden user-provided identifiers.
Adds basic functionality and tests for end-to-end sample generation.
Includes tests for comprehensive sample generation
Conduct translation from special variable '$resp' to 'response' in
template rendering
Includes macros to set up method invocation, adds code to include
license and copyright information,
Includes minor tweaks to various names, tests, and macros.
Note: sample generation is still ignorant of method and message
signatures, types, attributes, and so forth.
TODOs mark these areas in the code.
More full-featured semantic analysis will be conducted
in subsequent PRs.