-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Speech api changes #267
Speech api changes #267
Conversation
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
Author: puneith <puneith@google.com>
@lesv PTAL as well. |
Current coverage is 44.59%@@ master #267 diff @@
==========================================
Files 77 78 +1
Lines 2164 2267 +103
Methods 0 0
Messages 0 0
Branches 148 154 +6
==========================================
- Hits 1012 1011 -1
- Misses 1121 1225 +104
Partials 31 31
|
// | ||
// Then set environment variable GOOGLE_APPLICATION_CREDENTIALS to the full path of that file. | ||
|
||
package com.google.cloud.speech.grpc.demos; |
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.
For new samples we have been using com.example
package instead of com.google
. This is to make it more clear to the user that they can change the package to match their own project.
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.
This structure already existed. I didn't mess up with the package name. Change it still?
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.
Sorry about pinging on the package name, it looks like whom ever reviewed the first version wasn't on the Java team. :(
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.
@lesv @tswast Its common to put company name in the package even samples. This is google code so why not? https://github.com/linkedin/api-get-started/tree/master/java/src/com/linkedin/sample
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.
See RFC 2606
For the README.md - is the whole SERVICE ACCOUNT stuff still required? |
@@ -73,20 +73,35 @@ note that the audio file must be in RAW format. You can use `sox` | |||
(available, e.g. via [http://sox.sourceforge.net/](http://sox.sourceforge.net/) | |||
or [homebrew](http://brew.sh/)) to convert audio files to raw format. | |||
|
|||
### Run the non-streaming client | |||
### Run the sync client |
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.
How does it work on Windows?
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.
Don't know, is that a blocker?
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.
@lesv we don't have the instructions anywhere otherwise and assuming users would have read everything before using this sample is not correct either. So we need them IMHO.
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.
Please add an issue for this.
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.
Issue for windows or that we don't have instructions anywhere else?
@@ -73,20 +73,35 @@ note that the audio file must be in RAW format. You can use `sox` | |||
(available, e.g. via [http://sox.sourceforge.net/](http://sox.sourceforge.net/) |
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.
README should mention that this is BETA software w/ a link to what that means: https://cloud.google.com/terms/launch-stages
It should also mention that this samples is for Advanced Users.
LGTM |
There should be some additional items for catchup after merge. |
Fixes #292 #268 #267 ☕️ This is the draft I do not know it is considered as good test design using base class I basically got the idea from the stack overflow answer: https://stackoverflow.com/questions/8295100/how-to-re-run-failed-junit-tests-immediately
Fixes #292 #268 #267 ☕️ This is the draft I do not know it is considered as good test design using base class I basically got the idea from the stack overflow answer: https://stackoverflow.com/questions/8295100/how-to-re-run-failed-junit-tests-immediately
Fixes #292 #268 #267 ☕️ This is the draft I do not know it is considered as good test design using base class I basically got the idea from the stack overflow answer: https://stackoverflow.com/questions/8295100/how-to-re-run-failed-junit-tests-immediately
Fixes #292 #268 #267 ☕️ This is the draft I do not know it is considered as good test design using base class I basically got the idea from the stack overflow answer: https://stackoverflow.com/questions/8295100/how-to-re-run-failed-junit-tests-immediately
@tswast Please review. This PR contains the changes due to Speech API changes, namely Sync, Async and Streaming.