-
Notifications
You must be signed in to change notification settings - Fork 69
[samplecode][2/3]Implement Stream.Bidi sample code #632
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
Conversation
...java/com/google/api/generator/gapic/composer/samplecode/ServiceClientSampleCodeComposer.java
Outdated
Show resolved
Hide resolved
...java/com/google/api/generator/gapic/composer/samplecode/ServiceClientSampleCodeComposer.java
Outdated
Show resolved
Hide resolved
5a2cfa8 to
a14cc0a
Compare
miraleung
left a comment
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 with comments addressed.
| // Create bidi stream variable expression and assign it with invoking client's bidi stream | ||
| // method. | ||
| // e.g. BidiStream<EchoRequest, EchoResponse> bidiStream = echoClient.chatCallable().call(); | ||
| TypeNode bidiStreamType = |
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.
Since this method is already named bidi, do the variable names still need bidi in them?
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 used bidiStream to match the original sample code syntax, but I am open to the variable name. A stream variable in server is stream, so I think it make sense use stream instead of bidiStream. Are you also preferring using stream instead?
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 it's for matching the variable name, then bidiStream LGTM.
| // Create bidi stream variable expression and assign it with invoking client's bidi stream | ||
| // method. | ||
| // e.g. BidiStream<EchoRequest, EchoResponse> bidiStream = echoClient.chatCallable().call(); | ||
| TypeNode bidiStreamType = |
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 it's for matching the variable name, then bidiStream LGTM.
While updating multiple dependencies in the BOM, it's ok to have inconsistent versions. But we want to ensure the consistent versioning when we make a release of the BOM. I copied the setting from https://github.com/googleapis/java-cloud-bom/blob/main/.github/workflows/full-convergence-check.yaml#L10
🤖 I have created a release \*beep\* \*boop\* --- ### [2.3.2](https://www.github.com/googleapis/java-core/compare/v2.3.1...v2.3.2) (2021-12-02) ### Dependencies * update dependency com.google.api:api-common to v2.1.1 ([#632](https://www.github.com/googleapis/java-core/issues/632)) ([6232ef6](https://www.github.com/googleapis/java-core/commit/6232ef6861e99b1d6afe85e16c4bf9b4ff5039a9)) * update dependency com.google.api.grpc:proto-google-common-protos to v2.7.0 ([#638](https://www.github.com/googleapis/java-core/issues/638)) ([6c87f49](https://www.github.com/googleapis/java-core/commit/6c87f490808a208b814fdd93ca5bbc7aa64f4882)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Implement sample code for Stream.Bidi