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

Make Speech client use autogen layer #1631

Merged
merged 3 commits into from
Sep 28, 2016
Merged

Make Speech client use autogen layer #1631

merged 3 commits into from
Sep 28, 2016

Conversation

jmdobry
Copy link
Contributor

@jmdobry jmdobry commented Sep 26, 2016 •
edited
Loading

My initial attempt at using the autogen layer in the handwritten layer. I have a concern:

It would appear that gax-nodejs does not translate camel case option names into the snake case names defined the proto. Where sampleRate worked before, thy tests for recognize and createRecognitionStream started failing until I switched it to sample_rate.

Other than that it seems to work fine.

I found some other gax-nodejs usability issues with pagination that don't apply here.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 26, 2016
@stephenplusplus stephenplusplus added the api: speech Issues related to the Speech-to-Text API. label Sep 26, 2016
@stephenplusplus
Copy link
Contributor

stephenplusplus commented Sep 26, 2016 •
edited
Loading

// @jmuk

Is it possible to update gax-nodejs to expect camelcase args instead of snake? JS convention is generally camelcase. These are the options we had to provide to grpc to enable this: https://github.com/GoogleCloudPlatform/google-cloud-node/blob/95881d2a83904be5f2e4ebc24ee93369db212ac4/packages/common/src/grpc-service.js#L755

stephenplusplus
stephenplusplus reviewed Sep 26, 2016
View reviewed changes
packages/speech/src/index.js
@@ -87,6 +88,7 @@ function Speech(options) {
};

common.GrpcService.call(this, config, options);
this.api = new v1beta1(options).speechApi();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

jmdobry
jmdobry commented Sep 26, 2016
View reviewed changes
packages/speech/src/index.js
@@ -87,6 +88,9 @@ function Speech(options) {
};

common.GrpcService.call(this, config, options);
this.api = {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

jmdobry
jmdobry commented Sep 26, 2016
View reviewed changes
packages/speech/src/index.js
@@ -87,6 +88,9 @@ function Speech(options) {
};

common.GrpcService.call(this, config, options);
this.api = {
speechApi: v1beta1(options).speechApi()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@stephenplusplus
Copy link
Contributor

Just adding the don't merge while we wait to see if we can stick to 🐫 💼 .

@stephenplusplus
Copy link
Contributor

I found some other gax-nodejs usability issues with pagination that don't apply here.

Anything to call out here, or are you going to file something somewhere else?

@jmdobry
Copy link
Contributor Author

jmdobry commented Sep 26, 2016

Regarding camel case, it looks like gax-nodejs might need to be updated to output code that passes { convertFieldsToCamelCase: true } here: https://github.com/GoogleCloudPlatform/google-cloud-node/blob/master/packages/speech/src/v1beta1/speech_api.js#L220

Regarding pagination, will file elsewhere.

@jmdobry jmdobry mentioned this pull request Sep 27, 2016
Closed
@stephenplusplus
Copy link
Contributor

There's a new release of gax out (0.7.0) which camelCases and upgrades to gRPC@1.0. I believe we're now just waiting on the API files to be generated.

@jmdobry
Copy link
Contributor Author

jmdobry commented Sep 28, 2016

I'll wait for that PR to be merged then.

@evancohen evancohen mentioned this pull request Sep 28, 2016
Closed
@stephenplusplus
Copy link
Contributor

The PR with the re-generated files has been merged.

stephenplusplus
stephenplusplus reviewed Sep 28, 2016
View reviewed changes
packages/speech/src/index.js
};

self.request(protoOpts, reqOpts, function(err, apiResponse) {
self.api.Speech.syncRecognize(config, foundFile, function(err, apiResponse) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@jmdobry
Copy link
Contributor Author

jmdobry commented Sep 28, 2016

Updated

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Sep 28, 2016
@stephenplusplus stephenplusplus added cla: yes This human has signed the Contributor License Agreement. and removed don't merge cla: no This human has *not* signed the Contributor License Agreement. labels Sep 28, 2016
@stephenplusplus stephenplusplus merged commit 0f8d832 into googleapis:master Sep 28, 2016
@jmdobry jmdobry deleted the gax-speech branch September 28, 2016 17:36
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 216b575 on jmdobry:gax-speech into 96b845b on GoogleCloudPlatform:master.

sofisl pushed a commit that referenced this pull request Jan 24, 2023
@jmdobry @stephenplusplus
Make Speech client use autogen layer (#1631)
01e430a
sofisl pushed a commit that referenced this pull request Jan 25, 2023
@jmdobry @stephenplusplus
Make Speech client use autogen layer (#1631)
1882d4a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenplusplus stephenplusplus stephenplusplus left review comments

Assignees
No one assigned
Labels
api: speech Issues related to the Speech-to-Text API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmdobry @stephenplusplus @coveralls @googlebot