-
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 snippet index #1121
feat: add snippet index #1121
Conversation
Here is the summary of changes. You are about to add 2 region tags.
This comment is generated by snippet-bot.
|
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 job and nice tests.
I'm a little unclear about the dataflow here, please correct me if this is wrong.
- A snippet is generated, either manually or by snippetgen
- We send the snippet text into snippet_index.Snippet, which parses it and adds line number tags to particular sections.
- (TODO) We update the generated documentation (and documentation generation code) to point to these particular regions as in-text assistance.
# This will be need to be re-structured when one method can have | ||
# more than one sample variant |
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.
Can you elaborate a little bit on what this would look like? Is the method dictionary going to have more keys with more descriptive names? It seems like this would have a fairly large effect on add_snippet
and get_snippet
.
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.
In the future, there will be "configured" samples (2+ variants that set different parameters on the request). Right now we're still in the exploratory phase for config based snippets so I punted on creating something more complex and went with a dictionary. I imagine a different data structure (and a re-write of methods in this class) might be needed once there's more knobs.
It seems like this would have a fairly large effect on add_snippet and get_snippet.
Yep, agreed, my hope is that jinja template code can stay relatively stable and rely on get_snippet(...)
to collect whatever variant of the sample it needs.
I'll update the comment in the code to elaborate a bit more as well!
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, just one ongoing question and two test concatenations.
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
I realized I didn't see this earlier, but yes, this is the planned flow! The library docstrings will use the "full snippet" (snippet minus region tags), but other tools consuming the snippets might use a different combination of sections. |
Follow up to #1121. **Main changes:** * When snippets are generated, snippet metadata is also generated. * If a method has a snippet, that snippet is included in the method docstring. **Other changes:** * Removed the method docstring in the code snippet file (line right below the method definition) since the same text is already in the comment block at the top of the file. * Removed the concept of a "standalone" sample. All generated samples are expected to be standalone. When someone wants a smaller portion of the sample (e.g., request initialization only) they should fetch it from the file by looking up the line numbers in the snippet metadata file. Other Notes: * ~It doesn't look like it's possible to do type annotations with `_pb2` types, so those are annotated as `Any`.~ It is possible to do mypy checking with https://github.com/dropbox/mypy-protobuf, but I think it will be easier make that change in a separate PR. * There are a lot of golden file updates, [this range of commits](https://github.com/googleapis/gapic-generator-python/pull/1129/files/872c156f5100f1de20631dd59d083206432db374) has _most_ of the generator and test changes.
This PR adds
snippet_metadata.proto
and another samplegen utils class to store the snippets so that they can be looked up by library template code.PRs to begin generating metadata and add the samples to the library docstrings will follow (I originally planned it for one PR, but the changeset was a bit too big).