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

Add basic Cloud Speech API endpoints #8

Merged
merged 4 commits into from
Dec 18, 2017
Merged

Conversation

alexebird
Copy link
Contributor

  • also fixed some compilation warnings.

@PhillippOhlandt
Copy link

Oh, just what I need. Having this merged, together with a new release would be nice @sashaafm :)

@sashaafm
Copy link
Collaborator

sashaafm commented Jun 7, 2017

Hello @PhillippOhlandt, I'm inclined to add this if the original contributor @alexebird squashes and adds a good commit message.

@PhillippOhlandt
Copy link

@sashaafm That sounds like a deal!

@PhillippOhlandt
Copy link

I just tested the base branch for this PR. I think it's good to set a very high HTTPoison timeout for the asyncrecognize function. Or better, let the developer pass it as an option.

@PhillippOhlandt
Copy link

I just saw, the PR uses the v1beta1 API version. There is an official, slightly different v1 now.
https://cloud.google.com/speech/reference/rest/v1/speech/longrunningrecognize

@alexebird
Copy link
Contributor Author

alexebird commented Jun 12, 2017 via email

@PhillippOhlandt
Copy link

@alexebird awesome! :)

also:
- get tests green
- add larger-than-default HTTPoision timeouts
@alexebird
Copy link
Contributor Author

@sashaafm @PhillippOhlandt How do the changes look?

@PhillippOhlandt
Copy link

@alexebird There is one thing in the example body you have to change. In the v1 API they changed the sampleRate key to sampleRateHertz. Otherwise I think it looks good.

I saw you put a fixed timeout of 50 seconds in the request functions. This works. I hotpatched that in my local version to that value too. I also created #10 to ask if we can make this somehow configurable.

@PhillippOhlandt
Copy link

Another thing we could consider is letting the user pass in a map as body instead of raw json. This makes the API for the user a bit cleaner. What do you think?

@alexebird
Copy link
Contributor Author

Hey @PhillippOhlandt, sorry for the radio silence. I'm hoping to pick up work on my Elixir project again soon and I intend to include this PR in that. I'll look into passing the body as a map, but if it's a large change I don't want to give in to that scope creep.

-bird

@PhillippOhlandt
Copy link

@alexebird I could live without the body as map. But the example body should be fixed to not confuse new users.

@PhillippOhlandt
Copy link

It would be nice if this PR could be merged or if the new changes from this repo could be added to @alexebird fork. I am currently using this fork and my project is broken on OTP20 because I can't pull in the newest version of json_web_token (garyf/json_web_token_ex#20) due to Poison version conflicts.

@alexebird
Copy link
Contributor Author

alexebird commented Nov 8, 2017 via email

@PhillippOhlandt
Copy link

@alexebird just the mix.lock has conflicts with the current branch. Shouldn't be that hard to resolve.

@alexebird
Copy link
Contributor Author

alexebird commented Nov 11, 2017

@PhillippOhlandt Got the conflict resolved.

@sashaafm However, I had to bump the Elixir version to 1.4.0 to get the build to pass. I want to get this merged. Is this version bump okay?

@sashaafm
Copy link
Collaborator

Yes this is ok

@sashaafm
Copy link
Collaborator

If it is ready for merging?

@alexebird
Copy link
Contributor Author

@sashaafm Merge Away!

@sashaafm sashaafm merged commit 3353da5 into Overbryd:master Dec 18, 2017
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