API V2 discontinued - Changes to migrate to V3 - #31
Conversation
wronglink
left a comment
There was a problem hiding this comment.
Need some minor code style fixes and I'd also simplify the API as translator methods combine both array and not array requests.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| self.assertIsInstance(t, dict) | ||
| self.assertIn('Translations', t) | ||
| ts = self.translator.translate_array2(['The answer lies in machine translation.'], 'en', 'es') | ||
| translations = [t['translations'][0]['text'] for t in ts] |
There was a problem hiding this comment.
I'd also like more if we check the full results in ts, not the only first one.
| 'texts': json.dumps(texts), | ||
| } | ||
| return self._translate('TranslateArray', params, lang_from, lang_to, | ||
| body = [ |
There was a problem hiding this comment.
As I can see from docs, there is no difference between Translate and TranslateArray methods now. Shouldn't we combine methods with _array suffix into one?
There was a problem hiding this comment.
| body= [ | |
| def_array(self, texts=[], lang_from=None, lang_to=None, contenttype='text/plain', category='general', include_alignment=False): | |
| json= [ | |
| {'Text' : text} fortextintexts | |
| ] | |
| returnself._translate('translate', json, lang_from, lang_to, contenttype, category, include_alignment) | |
| deftranslate_array(self, texts=[], lang_from=None, lang_to=None, | |
| contenttype='text/plain', category='general'): | |
| returnself._array(self, texts, lang_from, lang_to, contenttype, category) | |
| deftranslate_array2(self, texts=[], lang_from=None, lang_to=None, | |
| contenttype='text/plain', category='general'): | |
| returnself._array(self, texts, lang_from, lang_to, contenttype, category, include_alignment=True) |
Do yo mean something like this?
| c = 0 | ||
| result = [] | ||
| for i in lengths: | ||
| for i in lengths[0]['sentLen']: |
There was a problem hiding this comment.
There are different names at doc page. Docs say it's sentLen and example says it's sentenceLengths. What is the actual behaviour?
There was a problem hiding this comment.
'sentLen' is the correct one, I think the docs example is wrong or has not been changed from a previous version.
| headers = { | ||
| 'Ocp-Apim-Subscription-Key': self.subscription_key | ||
| 'Ocp-Apim-Subscription-Key': self.subscription_key, | ||
| 'Content-type': 'application/json' |
There was a problem hiding this comment.
It's actually not required as we use json= parameter in requests. But if we keep it here, it would be better to leave a comma at the end of string:'Content-type': 'application/json',
There was a problem hiding this comment.
Not sure about this, I'm not used to use Python, why a comma at the end?
There was a problem hiding this comment.
It's a good habit as python allows to use trailing comma for cleaner diffs. If there was a trailing comma at after self.subscription_key before, we would see only one line added (not one changed and one added).
There was a problem hiding this comment.
Ok, I see the point. I have just added.
Uh oh!
There was an error while loading. Please reload this page.
martarodriguezm
commented
Feb 26, 2019
Hi, Have you had time to review the changes I submitted? Thanks! |
cantero6
commented
Apr 16, 2019
Hi, Could be possible to accept those changes so the Translator Text API can continue working? Thank you! |
Translator Text API V2 is going to be discontinued, I have done the migration to V3.
This Pull Request fixes#30