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

Support version 3 of translator API #3

Merged
merged 11 commits into from
Jun 7, 2019

Conversation

newearthmartin
Copy link
Contributor

@newearthmartin newearthmartin commented May 10, 2017

Support for version 3 of microsoft translator API
Update to using Azure cognitive services auth token
Fix tests (note: tests fail in travis because an azure token is needed)

NOTE: this PR started for version 2 of the API and then for version 3

@sharoonthomas
Copy link
Contributor

Any idea why the test is failing?

@newearthmartin
Copy link
Contributor Author

Hi! yes, but first let me tell something.

I've been taking a closer look at the code to implement V3. I can see that in this change both V1 and V2 are coexisting in parallel structures class Translator and class TranslatorCS(Translator). V1 is no longer supported so there is a lot of code that is not really needed. Also mentions Bing code that was already deprecated.

About tests, they fail for two reasons:

  • First, environment variables are not set. This means that one needs to set CLIENT_ID and CLIENT_SECRET to run the tests.
  • Second, and most important, the tests are done for V1 and obviously are failing because now only V2 is supported.

You can run the tests in your local machine and see this

$export CLIENT_ID=...
$export CLIENT_SECRET=...
$export PYTHONPATH=...
$python setup.py test

I don't know if we can get a test CLIENT_ID and CLIENT_SECRET for tests to run on travis, since these are linked to a billing account in Azure. AFAIK there are no "free only, never charge" accounts.

Conclusion:

This version works for V2. I'm currently working on V3 and will remove much code and leave it much more simple. We can wait until I finish that and merge it directly. Tests will not work for reason #1 but I can confirm that they work in my local machine.

Thoughts?

@newearthmartin
Copy link
Contributor Author

Hey @sharoonthomas !
I just pushed changes for V3 in this pull request

Reflecting changes in the API, here are some changes in the way the library works:

  • translate and translate_array have been merged into one method 'translate' that takes an array of texts.
  • They also can take many languages to translate at once, so the destination language can be 'en' or 'en,es'
  • So, return value is an array of arrays. It can be a little cumbersome for translating a single phrase to a singe language [['hola']] but it makes sense when translating multiple phrases to multiple languages [['Hallo', 'Ciao'], ['Auf Wiedersehen', 'Arrivederci']]
  • Also get_languages and detect API methods return more info.

So

  • I updated the README with usage and examples for every method. You can see it live in my repo https://github.com/newearthmartin/Microsoft-Translator-Python-API
  • Tests are fully working in the local machine. You need to define the environment variable AZURE_TRANSLATOR_KEY. This is the reason why they dont work in travis-ci.
  • I removed code from V1 and V2 so the code is much simpler now.
  • I took the liberty to update the library version to 0.9 hopefully to make a new release in pip 😃

What I didn't do is test it in python 3. Maybe tomorrow but also it would be nice that you test it in your local machine and confirm that tests work for you too.

Ok, looking forward to your review!

@newearthmartin newearthmartin changed the title Develop Support version 3 of translator API Feb 8, 2019
@newearthmartin
Copy link
Contributor Author

Now I tested in python 2 and python 3 and should be ready to merge

@sharoonthomas
Copy link
Contributor

Merging this. Can you also DM me a key that we can set on travis for automatic builds?

@newearthmartin
Copy link
Contributor Author

newearthmartin commented Feb 14, 2019 via email

@newearthmartin
Copy link
Contributor Author

After you merge this you can close this PR #2 which is included in this one

Also it would be awesome if you guys could make a pip release

@newearthmartin
Copy link
Contributor Author

hey! wondering whats the status on this one!
As I said before, I would love to see the tests running online but for that we need an azure key and it would be crazy to post an azure key linked to a credit card to an open source public repo.

@newearthmartin
Copy link
Contributor Author

hi @sharoonthomas I would love to see this merged so this library becomes useful again. Anything blocking the merge? If you guys dont want to maintain this anymore I can also take that role because my company uses this library

@sharoonthomas sharoonthomas merged commit 49797e5 into fulfilio:develop Jun 7, 2019
@newearthmartin
Copy link
Contributor Author

newearthmartin commented Jun 8, 2019

amazing!!!
looking forward to seeing this in pip and this library coming back to life again

You can now close this issue #1
And this PR is no longer needed #2

Thanks!

@newearthmartin
Copy link
Contributor Author

You can now close this issue #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants