-
Notifications
You must be signed in to change notification settings - Fork 24
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 snippet metadata file #1084
Conversation
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 regarding the generation end result!
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 as before, just forgot to add a couple of nits.
"clientLibrary": { | ||
"name": "nodejs-storage", | ||
"version": "0.1.0", | ||
"language": 9, |
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.
nit: Can you use the Enum value name instead, for readability mostly.
@@ -0,0 +1,187 @@ | |||
{ |
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.
If you can include the API version on he file name, that'd be great!
@amanda-tarafa, would you mind taking another look, I added in the points you mentioned in your email. Thanks again! |
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've left some comments that will apply to all the metadata files. Some thing might be different for Node to what I'm imagining, and if that's the case, then that's fine.
"language": "TYPESCRIPT", | ||
"apis": [ | ||
{ | ||
"id": "Addresses", |
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 should be the proto package.
I'm guessing this is really just the Compute API?, and maybe they should all go together in a single file?
The same as the file name, it should be snippet_metadata_{protopackage}.json.
Or what am I missing?
"origin": "API_DEFINITION", | ||
"description": " Retrieves an aggregated list of addresses.", | ||
"canonical": true, | ||
"file": "addresses.aggregated_list.js", |
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 double checking that the path of this file should be relatatice to this metadata file location. So, in this case it means that the metadata file and the snippet file are all in the same folder, which is totally fine
], | ||
"clientMethod": { | ||
"shortName": "AggregatedList", | ||
"fullName": "google.cloud.compute.v1.AggregatedList", |
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.
Wondering if this should be:
"fullName": "google.cloud.compute.v1.AggregatedList", | |
"fullName": "google.cloud.compute.v1.AddressesClient.AggregatedList", |
}, | ||
"method": { | ||
"shortName": "AggregatedList", | ||
"fullName": "google.cloud.compute.v1.AggregatedList", |
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 one should be as follows, since it's the proto defined method, which is defined on the service:
"fullName": "google.cloud.compute.v1.AggregatedList", | |
"fullName": "google.cloud.compute.v1.Addresses.AggregatedList", |
"version": "v1" | ||
}, | ||
{ | ||
"id": "RegionOperations", |
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.
And not sure why this one is here in any case.
No region tags are edited in this PR.This comment is generated by snippet-bot.
|
@amanda-tarafa, thank you for your review! Edited for your comments, PTAL, thanks again! |
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.
Yep, LGTM, thanks!!
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.
Great work 🎉 I like how you pulled in Amanda's proto to help ensure our implementation was correct 😄
return {clientLibrary, snippets}; | ||
} | ||
|
||
function createSnippetMetadata( |
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.
Idea for future refactor, it might be nice to pull the logic for generating the snippet index and individual snippet metadata into its own helper file.
🤖 I have created a release *beep* *boop* --- ## [2.13.0](v2.12.2...v2.13.0) (2022-02-03) ### Features * add snippet metadata file ([#1084](#1084)) ([5965e9c](5965e9c)) ### Bug Fixes * close client with stream methodshould return stream object ([#1101](#1101)) ([40f23d1](40f23d1)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Phase 3 of snippet generation, first draft