Skip to content
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

Merged
merged 10 commits into from
Jan 10, 2022
Merged

feat: add snippet index #1121

merged 10 commits into from
Jan 10, 2022

Conversation

busunkim96
Copy link
Contributor

@busunkim96 busunkim96 commented Jan 5, 2022

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).

@snippet-bot
Copy link

snippet-bot bot commented Jan 5, 2022

Here is the summary of changes.

You are about to add 2 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

Copy link
Contributor

@software-dov software-dov left a 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.

  1. A snippet is generated, either manually or by snippetgen
  2. We send the snippet text into snippet_index.Snippet, which parses it and adds line number tags to particular sections.
  3. (TODO) We update the generated documentation (and documentation generation code) to point to these particular regions as in-text assistance.

gapic/samplegen_utils/snippet_index.py Outdated Show resolved Hide resolved
gapic/samplegen_utils/snippet_index.py Outdated Show resolved Hide resolved
Comment on lines 100 to 101
# This will be need to be re-structured when one method can have
# more than one sample variant
Copy link
Contributor

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.

Copy link
Contributor Author

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!

gapic/samplegen_utils/snippet_metadata.proto Show resolved Hide resolved
tests/unit/samplegen/test_snippet_index.py Outdated Show resolved Hide resolved
@busunkim96 busunkim96 marked this pull request as ready for review January 6, 2022 20:39
@busunkim96 busunkim96 requested review from a team as code owners January 6, 2022 20:39
Copy link
Contributor

@software-dov software-dov left a 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.

gapic/samplegen_utils/snippet_metadata.proto Show resolved Hide resolved
gapic/samplegen_utils/snippet_metadata.proto Show resolved Hide resolved
tests/unit/samplegen/test_snippet_index.py Outdated Show resolved Hide resolved
Copy link

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

gapic/samplegen_utils/snippet_metadata.proto Show resolved Hide resolved
@busunkim96
Copy link
Contributor Author

  1. A snippet is generated, either manually or by snippetgen
  2. We send the snippet text into snippet_index.Snippet, which parses it and adds line number tags to particular sections.
  3. (TODO) We update the generated documentation (and documentation generation code) to point to these particular regions as in-text assistance.

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.

@busunkim96 busunkim96 merged commit 55d2bc6 into master Jan 10, 2022
@busunkim96 busunkim96 deleted the generate-snippet-index branch January 10, 2022 15:56
busunkim96 added a commit that referenced this pull request Jan 18, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants