-
Notifications
You must be signed in to change notification settings - Fork 66
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 proper handling of query/path/body parameters for rest transport #702
Merged
Merged
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
c1a6f89
feat: add rest transport generation for clients
yon-mg bd7c32d
feat: add rest transport generation for clients
yon-mg cd3a7f1
Merge branch 'master' of github.com:yon-mg/gapic-generator-python
yon-mg b41db31
feat: add transport flag
yon-mg ce69e7d
refactor: moved template logic outside
yon-mg 89fa08b
fix: small fixes in transport option logic
yon-mg 86f3a9c
test: added unit test for transport flag
yon-mg 234cc09
test: add unit test for http option method
yon-mg 35ac276
test: add unit test for http option method branch
yon-mg c8155a5
fix: fix import paths
yon-mg 55a815c
fix: style check issues
yon-mg ac0bdef
fix: more style check issues
yon-mg e79e8c7
fix: addressing pr reviews
yon-mg 824ad11
fix: typo in test_method
yon-mg 2d11e19
fix: style check fixes
yon-mg b5c6d06
Merge branch 'master' into master
yon-mg 221cd44
feat: add proper handling of query/path/body parameters for rest tran…
yon-mg 475f903
Merge branch 'master' of github.com:yon-mg/gapic-generator-python
yon-mg f08ca32
Merge remote-tracking branch 'upstream/master'
yon-mg efa0ed6
fix: typing errors
yon-mg d3e08c0
Update case.py
software-dov df88ef9
fix: minor changes adding a test, refactor and style check
yon-mg 67c6bd1
Merge branch 'master' of github.com:yon-mg/gapic-generator-python
yon-mg 37e3d28
fix: camel_case bug with constant case
yon-mg c964fba
fix: to_camel_case to produce lower camel case instead of PascalCase …
yon-mg 85be88d
fix: addressing pr comments
yon-mg 5d1c6a3
fix: adding appropriate todos, addressing comments
yon-mg f6c64cc
fix: dataclass dependency issue
yon-mg a4f34e7
Update wrappers.py
software-dov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -168,13 +168,16 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): | |
#} | ||
# TODO(yon-mg): handle nested fields corerctly rather than using only top level fields | ||
# not required for GCE | ||
potentialParams = { | ||
query_params = { | ||
{%- for field in method.query_params %} | ||
'{{ field|camel_case }}': request.{{ field }}, | ||
vchudnov-g marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{%- endfor %} | ||
} | ||
potentialParams = ((k, v) for k, v in potentialParams.items() if v) | ||
for i, (param_name, param_value) in enumerate(potentialParams): | ||
# TODO(yon-mg): further discussion needed whether 'python truthiness' is appropriate here | ||
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. Thanks for the TODO on truthiness! |
||
# discards default values | ||
# TODO(yon-mg): add test for proper url encoded strings | ||
query_params = ((k, v) for k, v in query_params.items() if v) | ||
for i, (param_name, param_value) in enumerate(query_params): | ||
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: Do consider the string.Join suggestion from earlier (but it's certainly not a blocker) |
||
q = '?' if i == 0 else '&' | ||
url += "{q}{name}={value}".format(q=q, name=param_name, value=param_value.replace(' ', '+')) | ||
vchudnov-g marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Just curious: why can't
query_params
andpath_params
return the same type, rather than aSet
and aSequence
?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.
I suppose they could. Just thought it's more appropriate since ordering seems important for
path_params
but not forquery_params
.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.
As discussed. That seems reasonable, but a complication is that repeated fields map to repeated query params. In that case, a sequence may be best. (If order doesn't matter, a multiset might be enough, but we can't assume that order doesn't matter for repeated fields)
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.
You are right set is not appropriate even if order doesn't matter but I should mention though that once query param logic is moved out, this won't matter anymore.
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.
True!