-
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 autogenerated snippets #845
Conversation
Here is the summary of possible violations 😱 There are 7 possible violations for not having product prefix.
The end of the violation section. All the stuff below is FYI purposes only. Here is the summary of changes. You are about to add 7 region tags.
You are about to delete 2 region tags.This comment is generated by snippet-bot.
|
Codecov Report
@@ Coverage Diff @@
## master #845 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 27 +1
Lines 1608 1694 +86
Branches 328 347 +19
=========================================
+ Hits 1608 1694 +86
Continue to review full report at Codecov.
|
Lol please ignore snippetbot, I'll add a config to ignore those paths. |
I'm honestly surprised as much of the original sample gen code is as reusable as seems to be the case. Neat! First pass looks great, going to look more closely in just a second. |
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 a few minor nits. Looks great!
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.
LGTM on the functional side of things.
I just left a comment re: region tags. Thanks!!
gapic/generator/generator.py
Outdated
for client_name, client in service.clients.items(): | ||
|
||
for rpc_name, method_list in client.rpcs.items(): | ||
# generated_${api}_${apiVersion}_${serviceName}_${rpcName}_{sync|async} |
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.
This is going to change now so that snippets can be matched to products by the snippet tracker. @busunkim96 I've cc'ed you on a comment on the desgin doc.
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.
Fixed! One thing I noticed is that the endpoint is declared at the service level. I guess in practice the endpoint is the same across all services in the same API? https://github.com/googleapis/googleapis/blob/ae5fb2884a296832c39867e8e8c81bbc72a32ce8/google/cloud/pubsublite/v1/subscriber.proto#L35
This PR targets iteration 1 and 2 specified in the Snippet Gen Design: Full canonical coverage of simple requests, paginated, LRO, server streaming, and Bidi streaming with empty request objects.
Snippet generation is hidden behind a new option
autogen-snippets
.After discussion with folks on different language teams on snippetgen, I decided using "golden" snippet files would be easier than following the unit testing strategy used to check the library surface. I also believe goldens will be be easier for review for other Python DPEs.
Other notes: