Overbryd / gcloudex

Friendly set of wrappers for Google Cloud Platform services' API's in Elixir.
64 stars 39 forks source link

Add basic Cloud Speech API endpoints #8

Closed alexebird closed 6 years ago

alexebird commented 7 years ago
PhillippOhlandt commented 7 years ago

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

sashaafm commented 7 years ago

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

PhillippOhlandt commented 7 years ago

@sashaafm That sounds like a deal!

PhillippOhlandt commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

I'll get this updated!

On Mon, Jun 12, 2017 at 6:43 AM Phillipp Ohlandt notifications@github.com wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sashaafm/gcloudex/pull/8#issuecomment-307793331, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHXZuuDPe0K7WytfnbEVZzfZYrT2P2zks5sDUCNgaJpZM4MpPpb .

PhillippOhlandt commented 7 years ago

@alexebird awesome! :)

alexebird commented 7 years ago

@sashaafm @PhillippOhlandt How do the changes look?

PhillippOhlandt commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

PhillippOhlandt commented 6 years ago

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(https://github.com/garyf/json_web_token_ex/issues/20) due to Poison version conflicts.

alexebird commented 6 years ago

Sorry, I haven’t been working on this project much lately but I will try to get my fork updated shortly.

On Wed, Nov 1, 2017 at 4:09 AM Phillipp Ohlandt notifications@github.com wrote:

It would be nice if this PR could be merged or if the new changes from this repo could be added to @alexebird https://github.com/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 https://github.com/garyf/json_web_token_ex/issues/20) due to Poison version conflicts.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sashaafm/gcloudex/pull/8#issuecomment-341077822, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHXZgLBQwwDYSElNPXO-91g658c5Aoyks5syFFQgaJpZM4MpPpb .

PhillippOhlandt commented 6 years ago

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

alexebird commented 6 years ago

@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 commented 6 years ago

Yes this is ok

sashaafm commented 6 years ago

If it is ready for merging?

alexebird commented 6 years ago

@sashaafm Merge Away!