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.

Sign up for GitHub

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

Jump to bottom

feat: add async samples #861

Merged
merged 8 commits into from
Jun 11, 2021
Merged

feat: add async samples #861

merged 8 commits into from
Jun 11, 2021

Conversation

busunkim96
Copy link
Contributor

No description provided.

@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label May 4, 2021
@snippet-bot
Copy link

snippet-bot bot commented May 4, 2021 •
edited
Loading

Here is the summary of changes.

You are about to add 127 region tags.
You are about to delete 63 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

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 4, 2021
@codecov
Copy link

codecov bot commented May 4, 2021 •
edited
Loading

Codecov Report

Merging #861 (7a7e2f1) into master (7c185e8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #861   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        27    +1     
  Lines         1608      1703   +95     
  Branches       328       347   +19     
=========================================
+ Hits          1608      1703   +95
Impacted Files Coverage Δ
gapic/samplegen_utils/types.py 100.00% <ø> (ø)
gapic/samplegen_utils/utils.py 100.00% <ø> (ø)
gapic/utils/case.py 100.00% <ø> (ø)
gapic/utils/reserved_names.py 100.00% <ø> (ø)
gapic/generator/generator.py 100.00% <100.00%> (ø)
gapic/samplegen/samplegen.py 100.00% <100.00%> (ø)
gapic/schema/api.py 100.00% <100.00%> (ø)
gapic/schema/metadata.py 100.00% <100.00%> (ø)
gapic/schema/wrappers.py 100.00% <100.00%> (ø)
gapic/utils/__init__.py 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25aa8c9...7a7e2f1. Read the comment docs.

@busunkim96 busunkim96 marked this pull request as ready for review May 24, 2021 16:07
@busunkim96 busunkim96 requested a review from a team as a code owner May 24, 2021 16:07
software-dov
software-dov reviewed Jun 9, 2021
View reviewed changes
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.

Looks good, just a few optional nits and what looks like a copy/paste error.
One other thing: I'm not sure I like transport types being magic strings. If I were making code additions in this area, I would definitely mess up at some point and use "grpc_async" or "grpcAsync" and not catch it immediately. How annoying would it be to use named constants instead?

gapic/samplegen/samplegen.py Outdated Show resolved Hide resolved
gapic/samplegen/samplegen.py Outdated Show resolved Hide resolved
tests/unit/samplegen/test_template.py Outdated Show resolved Hide resolved
tests/unit/samplegen/test_samplegen.py Outdated Show resolved Hide resolved
@busunkim96
Copy link
Contributor Author

@software-dov Thank you for the review! Will clean this up and re-request a review.

One other thing: I'm not sure I like transport types being magic strings. If I were making code additions in this area, I would definitely mess up at some point and use "grpc_async" or "grpcAsync" and not catch it immediately. How annoying would it be to use named constants instead?

Named constants sound like they will be less error prone :)

@busunkim96 busunkim96 requested a review from software-dov June 9, 2021 22:58
@busunkim96
Copy link
Contributor Author

@software-dov PTAL.

I'm not sure why codecov isn't running. Is there a way to trigger it manually? Should I try pushing another commit?

@software-dov
Copy link
Contributor

I don't know why codecov isn't running. The tubes must be clogged.
I think the best way to try to solve this is to just rerun all the CI. That's easy to do from the dashboard.

@software-dov software-dov merged commit e385ffd into master Jun 11, 2021
@software-dov software-dov deleted the async-samples branch June 11, 2021 17:55
gcf-merge-on-green bot pushed a commit that referenced this pull request Jun 11, 2021
@release-please
chore: release 0.49.0 (#919)
1e39f41 
🤖 I have created a release \*beep\* \*boop\*
---
## [0.49.0](https://www.github.com/googleapis/gapic-generator-python/compare/v0.48.1...v0.49.0) (2021-06-11)


### Features

* add async samples ([#861](https://www.github.com/googleapis/gapic-generator-python/issues/861)) ([e385ffd](https://www.github.com/googleapis/gapic-generator-python/commit/e385ffd7f012c6a38c9fcd7c5f36ce090311032b))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This was referenced Jan 18, 2022
Merged
Closed
This was referenced Jan 21, 2022
Closed
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@software-dov software-dov software-dov approved these changes

@tseaver tseaver tseaver approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@busunkim96 @software-dov @tseaver