Translating sentences with an API wrapper 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'))
    # =&gt; Beautiful is better than ugly
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.