5
\$\begingroup\$

I've written a wrapper for this handy Yandex Translate API. I am pretty happy with is as it works just fine and the translations are accurate, even translations of entire sentences. However, I am curious if there is a better way to put the wrapper together, what would you do differently and what could be improved? I'll take any remarks there is.

import requests
from requests.exceptions import ConnectionError


class YandexException(Exception):
    """Common API wrapper exception."""
    pass


class Yandex:
    """YandexTranslate API wrapper."""

    API_VERSION = 'v1.5'
    URL = 'https://translate.yandex.net/api/v1.5/tr.json/'

    def __init__(self, api_key):
        """Use an API key provided by Yandex.
        To obtain a key, visit https://translate.yandex.com/developers
        """
        self.api_key = api_key
        self._session = requests.Session()

    @property
    def supported_languages(self):
        """Return a dictionary of currently supported languages.
        All languages are represented using ISO 639-1 language codes.
        """
        payload = {
            'ui': 'en',
            'key': self.api_key
        }
        try:
            response = self._session.get(self.URL + 'getLangs', params=payload)
            if response.status_code != 200:
                raise YandexException(response.json())
            return response.json().get('langs')
        except ConnectionError:
            raise YandexException('API unavailable')

    def _get_translate_direction(self, source, target):
        """Raise an exception if source and target language are not currently
        supported by the Yandex Translate API.
        """
        if source and target in self.supported_languages:
            return '{}-{}'.format(source, target)
        if source is None and target in self.supported_languages:
            return target
        raise YandexException('language direction not supported')

    def translate(self, text, target='en', source=None):
        """Return translated text.

        Translated text may be inaccurate if source language is not specified.
        This is because the API will try to detect source language, if the
        relevant argument is None.

        Text will be translated to English by default.

        Only use ISO 639-1 language codes with this method.
        """
        translate_direction = self._get_translate_direction(source, target)
        payload = {
            'text': text,
            'format': 'plain',
            'lang': translate_direction,
            'key': self.api_key
        }
        try:
            response = self._session.post(self.URL + 'translate', data=payload)
            if response.status_code != 200:
                raise YandexException(response.json())
            return response.json().get('text')[0]
        except ConnectionError:
            raise YandexException('API unavailable')

    def detect_language(self, text):
        """Return language of passed in text as a ISO 639-1 language code."""
        payload = {
            'text': text,
            'format': 'plain',
            'key': self.api_key
        }
        try:
            response = self._session.post(self.URL + 'detect', data=payload)
            if response.status_code != 200:
                raise YandexException(response.json())
            return response.json().get('lang')
        except ConnectionError:
            raise YandexException('API unavailable')

if __name__ == '__main__':
    t = Yandex(api_key)
    print(t.translate('Piękne jest lepsze niż brzydsze'))
    # => Beautiful is better than ugly
\$\endgroup\$
5
\$\begingroup\$

Dobra robota, tak trzymaj.

Your code is rather good code:

  • Docstrings, yay;
  • PEP 8 compliant (except a missing second blank line before if __name__ == '__main__');
  • Custom exception for the module;
  • Not executing top-level code on imports;
  • Sensible defaults;
  • Readable.

However, it's rather WET and you've got a silly rookie mistake.

Bug

Reading the docstring above, I'm pretty sure if source and target in self.supported_languages: is not checking what you intended to check.

This is parsed as if source and (target in self.supported_languages): which checks for the truth value of source rather than its existence in the self.supported_language dictionary.

Instead you could write:

def _get_translate_direction(self, source, target):
    """Raise an exception if source and target language are not currently
    supported by the Yandex Translate API.
    """
    if target not in self.supported_languages:
        raise YandexException('language target \'{}\' not supported'.format(target))
    if source is None:
        return target

    if source not in self.supported_languages:
        raise YandexException('language source \'{}\' not supported'.format(source))
    return '{}-{}'.format(source, target)

DRY it up

Each of the 3 method communicating with the API reuse the same exact structure and mostly share 7 lines in common. You could extract that to simplify things:

def _get_api_response(self, endpoint, payload):
    try:
        response = self._session.get(self.URL + endpoint, params=payload)
        if response.status_code != 200:
            raise YandexException(response.json())
        return response.json()
    except ConnectionError:
        raise YandexException('API unavailable')

@property
def supported_languages(self):
    payload = {
        'ui': 'en',
        'key': self.api_key
    }
    return self._get_api_response('getLangs', payload).get('langs')

def translate(self, text, target='en', source=None):
    translate_direction = self._get_translate_direction(source, target)
    payload = {
        'text': text,
        'format': 'plain',
        'lang': translate_direction,
        'key': self.api_key
    }
    return self._get_api_response('translate', payload).get('text')[0]

def detect_language(self, text):
    payload = {
        'text': text,
        'format': 'plain',
        'key': self.api_key
    }
    return self._get_api_response('detect', payload).get('lang')

Improvements

I’m not sure why you chose to .get elements out of the JSON response rather than accessing them directly with ['lang'] or ['text']: in case the API doesn't respond with the expected JSON, you won't figure it out. Worse, it will crash on other parts of the code with no obvious reasons.

For instance, if the getLangs call does not return a dictionary with a 'langs' key, response.json().get('langs') will return None. This will make the checks target in self.supported_languages raise TypeError because None is not iterable. This is not ideal. You should either:

  • access the key directly and catch the KeyError to throw your own exception;
  • or use a default value in your .get calls that is compatible with what is expected later.

For the second option, I'd use:

return self._get_api_response('getLangs', payload).get('langs', {})
return self._get_api_response('translate', payload).get('text', [None])[0]
return self._get_api_response('detect', payload).get('lang')

For the last two, None seems valid as it convey the "I couldn't detect/translate" meaning.


An other improvement I would make is to cache the returned value of supported_languages as there is very few chances that it changes during the lifespan of the application. I’d use functools.lru_cache for that:

from functools import lru_cache


class Yandex:
    ...
    @property
    @lru_cache(maxsize=1)
    def supported_languages(self):
        payload = {
            'ui': 'en',
            'key': self.api_key
        }
        return self._get_api_response('getLangs', payload).get('langs', {})

I chose a maxsize of 1 because there is little to no chance that more than 1 Yandex object get instanciated in a given application.


Lastly, I’m not aware if Yandex provide a sandboxed environment with some kind of public key that anyone can use with restrictions on the amount of requests, for instance. But if they do, it could be a great addition as a default value for the constructor.

\$\endgroup\$
3
  • \$\begingroup\$ When it comes to pulling elements out of the JSON response, I figured only response 200 will have the correct element. Before the JSON can be accessed, the exception should take care of preventing bad output since the response wouldn't have the status code of 200. \$\endgroup\$ – Lukasz Sep 28 '17 at 20:34
  • \$\begingroup\$ lru_cachce is quite literally a godsent, I've been thinking of a way to persist supported_languages other than putting it into the constructor. Speaking of the constructor, Yandex unfortunately doesn't provide public keys - I'm guessing it's their take of preventing abuse of the service. Nice touch with the first sentence, appreciate the review! \$\endgroup\$ – Lukasz Sep 28 '17 at 20:35
  • \$\begingroup\$ @LukaszSalitra Oh yes, having self.supported_languages = self._get_api_response('getLangs', payload).get('langs', {}) in the constructor might be even more robust than using lru_cache \$\endgroup\$ – 301_Moved_Permanently Sep 29 '17 at 10:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.