6
\$\begingroup\$

I wrote this little class to wrap around a work-in-progress, poorly documented API and I'd like to know what else can be improved. I feel like there's a lot of repetition in the functions and I'd love to know if I could turn all those functions (which do the same thing, just setting a different command in the request object) into just one.

import json
import requests

BASE_URL = "https://chaos.aa.net.uk/"

class Chaos(object):
    """This class allows access to the Andrews & Arnold API.

    Note that it is based on trial and error, there is very little
    official documentation about this API yet, so use at your own risk.
    """

    def __init__(self, username, password):
        """Initialize the class.

        Use the same credentials as on control.aa.net.uk.

        Args:
            username: username like xx00@x
            password: self-explanatory
        """
        self.session = requests.session()
        self.session.headers["User-Agent"] = "Python Chaos Client"
        self.session.auth = (username, password)

    def _request(self, **kwargs):
        """Make an API request, lets Requests check the HTTP status code
        then checks if the "error" string is present in the response
        and raises an exception if that's the case.

        Args:
            **kwargs: will be passed as-is to python-requests
        Returns:
            a dict representation of the APi'S JSON reply
        Raises:
            Exception: the remote server returned an error
        """

        resp = self.session.post(BASE_URL, **kwargs)

        if resp.status_code != requests.codes.ok:
            resp.raise_for_status()

        resp = resp.json()

        if "error" in resp:
            raise APIError(resp["error"])

        return resp

    def info(self, **kwargs):
        return self._request(json={**kwargs, **{"command": "info"}})

    def change(self, **kwargs):
        required = ["broadband", "sim", "voip"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={**kwargs, **{"command": "change"}})

    def check(self, **kwargs):
        required = ["order"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={**kwargs, **{"command": "check"}})

    def preorder(self, **kwargs):
        required = ["order"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={**kwargs, **{"command": "preorder"}})

    def order(self, **kwargs):
        required = ["order"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={**kwargs, **{"command": "order"}})

    def usage(self, **kwargs):
        required =  ["broadband", "sim", "voip"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(allowed))

        return self._request(json={**kwargs, **{"command": "usage"}})

    def availability(self, **kwargs):
        required = ["broadband"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={**kwargs, **{"command": "availability"}})

class ChaosException(Exception):
    """Base class for all our exceptions.
    """
    pass

class InvalidParameters(ChaosException):
    """Indicates a problem with the request's arguments.
    """
    pass

class APIError(ChaosException):
    """Indicates an error returned by the remote server.
    """
    pass
\$\endgroup\$
3
\$\begingroup\$

This is to extend @dpercy's answer a bit. If you make the required fields an attribute of your class you could override __getattr__ to give you access to all the functions without actually having to write them:

from functools import partial


class Chaos(object):
    required = {"info": [], "change": ["broadband", "sim", "voip"]}

    def __init__(self):
        # actual implementation here
        pass

    def _command(self, command_name, required, **kwargs):
        # implement calling _request with these arguments
        print command, required, kwargs

    def __getattr__(self, name):
        try:
            return partial(self._command, name, Chaos.required[name])
        except KeyError as e:
            raise AttributeError("'Chaos' object has no attribute {}".format(e))

This can be used like:

>>> c = Chaos()
>>> c.info
<functools.partial object at 0x7f081a70baf8>
>>> c.info()
info [] {}
>>> c.change()
change ['broadband', 'sim', 'voip'] {}
>>> c.info(x=3)
info [] {'x': 3}

It works, because __getattr__ is called if no other attribute with that name is found. The attribute getting from the class itself is also not overwritten (meaning, you have to put the requirements in th class itself, in the instance it would cause an infinite recursion...).

EDIT1: Be aware, though, that the dictionary of required is shared between all instances of Chaos, therefore this might lead to a bug if you have multiple instances and try to modify them on-the-fly just for that instance:

>>> a = Chaos()
>>> b = Chaos()
>>> a.required['foo'] = []
>>> b.foo()
foo [] {}

EDIT2: As an alternative, to avoid the above problem and speed things up a bit, you could assign these functions at instance creation, using setattr:

class Chaos(object):
    required = {"info": [], "change": ["broadband", "sim", "voip"]}

    def __init__(self):
        for name, required in Chaos.required.items():
            setattr(self, name, partial(self._command, name, required))

    def _command(self, command, required, **kwargs):
        print command, required, kwargs

This should be faster (about 4 times on my machine), especially if you call the methods many times, because partial is only called once and not every time. It also means that commands like vars(c) and dir(c) now contain the defined functions.

EDIT3: Now this code has no documentation of these functions. This can be remedied partially with:

class Chaos(object):
    required = {"info": [], "change": ["broadband", "sim", "voip"]}

    def __init__(self):
        docstr = "Wrapper for '{}._command(command='{}', required={}, **kwargs)"
        for name, required in Chaos.required.items():
            setattr(self, name, partial(self._command, name, required))
            getattr(self, name).__doc__ = docstr.format(self, name, required)

    def __str__(self):
        return "Chaos"

    def _command(self, command, required, **kwargs):
        print command, required, kwargs

This way, you can at least do:

>>> c.info.__doc__
"Wrapper for 'Chaos._command(command='info', required=[], **kwargs)"

But not:

>>> help(c.info)
<Help page of 'functools.partial'>
\$\endgroup\$
  • \$\begingroup\$ @Andr See edited answer for a slightly faster way to do it. \$\endgroup\$ – Graipher Aug 18 '16 at 13:52
5
\$\begingroup\$

You're right, you can cut down on the repetition of code. Although bundling all of the functions into one like you mention would work, you lose a bit of the readability that keeping them as class methods gives. Instead, you could just have an internal _command method that collects all of the repeated code:

def _command(self, command_name, required, **kwargs):
    if not any(arg in required for arg in kwargs):
        raise InvalidParameters("Missing object of types: " + ", ".join(required))
    return self._request(json={kwargs, {"command": command_name }})

Note that I've corrected a syntax error in the last line: your parameter used to read json={**kwargs, **{"command": command_name }}.

This makes your Chaos class a lot shorter:

class Chaos(object):
    """This class allows access to the Andrews & Arnold API.

    Note that it is based on trial and error, there is very little
    official documentation about this API yet, so use at your own risk.
    """

    def __init__(self, username, password):
        """Initialize the class.

        Use the same credentials as on control.aa.net.uk.

        Args:
            username: username like xx00@x
            password: self-explanatory
        """
        self.session = requests.session()
        self.session.headers["User-Agent"] = "Python Chaos Client"
        self.session.auth = (username, password)

    def _request(self, **kwargs):
        """Make an API request, lets Requests check the HTTP status code
        then checks if the "error" string is present in the response
        and raises an exception if that's the case.

        Args:
            **kwargs: will be passed as-is to python-requests
        Returns:
            a dict representation of the APi'S JSON reply
        Raises:
            Exception: the remote server returned an error
        """

        resp = self.session.post(BASE_URL, **kwargs)

        if resp.status_code != requests.codes.ok:
            resp.raise_for_status()

        resp = resp.json()

        if "error" in resp:
            raise APIError(resp["error"])

        return resp

    def _command(self, command_name, required, **kwargs):
        """Make an API request, checking that the arguments are valid

        Args:
            command_name: the command name to be passed to the API
            required: a list of names of required arguments
            **kwargs: will be passed as-is to python-requests
        Returns:
            a dict representation of the APi'S JSON reply
        Raises:
            InvalidParameters: the wrong set of arguments was given
        """

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={kwargs, {"command": command_name }})


    def info(self, **kwargs):
        return self._command('info', [], **kwargs)

    def change(self, **kwargs):        
        return self._command('change', ["broadband", "sim", "voip"], **kwargs)

    def check(self, **kwargs):
        return self._command('check', ["order"], **kwargs)

    def preorder(self, **kwargs):
        return self._command('preorder', ["order"], **kwargs)

    def order(self, **kwargs):
        return self._command('order', ["order"], **kwargs)

    def usage(self, **kwargs):
        return self._command('usage', ["broadband", "sim", "voip"], **kwargs)        

    def availability(self, **kwargs):
        return self._command('usage', ["broadband"], **kwargs)

Since _request only ever gets called by _command you could compress this further by making them one function, but it's fine as is.

As a side note, this code only checks that the minimum set of arguments is given. Depending on your API, it might be a problem if you give extra arguments that aren't required or supported.

\$\endgroup\$
  • \$\begingroup\$ I'm not sure which one to accept because both answers are awesome, but I do prefer the __getattr___-based solution from the other answer. In any case, +1 and thank you. \$\endgroup\$ – André Borie Aug 18 '16 at 0:03
4
\$\begingroup\$

A couple of comments on requests usage:

if resp.status_code != requests.codes.ok:
    resp.raise_for_status()

Here checking that status code is not 200 won't do anything, because raise_for_status raises an error only for error codes (like 4xx or 5xx).

See http://docs.python-requests.org/en/master/_modules/requests/models/#Response.raise_for_status


resp = resp.json()

Here you might want to handle JsonDecodingError (or smth. like that) in case response content is not a JSON, which happens.

\$\endgroup\$

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.