patternpythonMinor
Python API client
Viewed 0 times
clientapipython
Problem
I'm creating a Python API client, after thinking and checking some open source Python clients I created this class, and I'm looking for any feedback (design, best practices...), this is an example URL of the API:
Here's my class using the requests library:
```
import requests
class APIClient(object):
"""Creates an API client object
:param userid: the API userid found in the control panel
:param api_key: the API key found in the control panel Settings > API
"""
def __init__(self, userid=None, api_key=None):
# userid and api key sent with every request
self.userid = userid
self.key = api_key
# the base production api url
self.base_url = None
# the sandbox api url
self.test_url = None
# boolean value to indicate which url to use
self.testing = False
# the error description if it occured
self.error = None
def request(self, method='GET', path=None, params=None):
"""Makes a request to the API with the given parameters
:param method: the HTTP method to use
:param path: the api path to request
:param params: the parameters to be sent
"""
# if we're testing use the sandbox url
api_url = self.test_url if self.testing else self.base_url
# add the authentication parameters (sent with every request)
params.update({
'userid': self.userid,
'key': self.key
})
# the api request result
result = None
try:
# send a request to the api server
r = requests.request(
method = method,
url = api_url + path + '.json',
params = params,
headers = {'User-Agent': 'Python API Client'}
)
# raise an exception if status code is not 2
https://www.api.com/collection/resource.json?userid=id&key=apikey&command=valueHere's my class using the requests library:
```
import requests
class APIClient(object):
"""Creates an API client object
:param userid: the API userid found in the control panel
:param api_key: the API key found in the control panel Settings > API
"""
def __init__(self, userid=None, api_key=None):
# userid and api key sent with every request
self.userid = userid
self.key = api_key
# the base production api url
self.base_url = None
# the sandbox api url
self.test_url = None
# boolean value to indicate which url to use
self.testing = False
# the error description if it occured
self.error = None
def request(self, method='GET', path=None, params=None):
"""Makes a request to the API with the given parameters
:param method: the HTTP method to use
:param path: the api path to request
:param params: the parameters to be sent
"""
# if we're testing use the sandbox url
api_url = self.test_url if self.testing else self.base_url
# add the authentication parameters (sent with every request)
params.update({
'userid': self.userid,
'key': self.key
})
# the api request result
result = None
try:
# send a request to the api server
r = requests.request(
method = method,
url = api_url + path + '.json',
params = params,
headers = {'User-Agent': 'Python API Client'}
)
# raise an exception if status code is not 2
Solution
-
Python has a mechanism for handling exceptional situations, but you go to great lengths to suppress it: you catch all the exceptions that might result from your
This has several problems.
First, it needlessly complicates every API call. A caller can't just write:
allowing exceptions to pass up the call stack and so eventually appear on the console or in the log. Instead, they have to write:
This means that every call to your API needs to consider how to handle errors. What are programmers going to do? Well, mostly likely they will raise these errors:
So why bother suppressing these errors in your API in the first place?
Second, you replace all exceptions that you don't recognize with
Third, you lose information that was present in the exception objects. For example, an
This kind of information is vital in tracking down the cause of problems. But you replace this with
-
If the status code is not 200, you:
Which has three problems: (i) you should raise an instance of an exception class, not the class itself; (ii) you should initialize the exception object with data that describes the exception; and (iii) the class
And then:
But in fact the
-
Your interface for setting API parameters is to set them directly as properties of the API object:
It would be better for the constructor to take these as keyword arguments: (i) this allows the constructor to check that all required parameters have been set; (ii) parameter values can be checked (if possible); and (iii) the keyword argument mechanism is more flexible than object properties since you can pass a sets of keywords around in the form of a dictionary. So I would expect to write:
Python has a mechanism for handling exceptional situations, but you go to great lengths to suppress it: you catch all the exceptions that might result from your
request method and replace them with a string in the .error property of the APIClient object.This has several problems.
First, it needlessly complicates every API call. A caller can't just write:
r = api.request(...) # might raise an exception
do_something_with(r)allowing exceptions to pass up the call stack and so eventually appear on the console or in the log. Instead, they have to write:
api.error = None # clear the old error, if any
r = api.request(...) # might set api.error
if api.error:
# handle the error somehow
else:
do_something_with(r)This means that every call to your API needs to consider how to handle errors. What are programmers going to do? Well, mostly likely they will raise these errors:
api.error = None # clear the old error, if any
r = api.request(...) # might set api.error
if api.error:
raise MyError("API error: {}".format(api.error))
else:
do_something_with(r)So why bother suppressing these errors in your API in the first place?
Second, you replace all exceptions that you don't recognize with
An unexpected error occurred. So this makes it impossible to distinguish a ProxyError from a TooManyRedirects from an SSLError.Third, you lose information that was present in the exception objects. For example, an
SSLError comes with a description of the problem, for example if the certificate doesn't match the domain, you'll get an error like this:requests.exceptions.SSLError: hostname 'foo.com' doesn't match 'bar.com'This kind of information is vital in tracking down the cause of problems. But you replace this with
An unexpected error occurred which is, frankly, useless.-
If the status code is not 200, you:
raise ExceptionWhich has three problems: (i) you should raise an instance of an exception class, not the class itself; (ii) you should initialize the exception object with data that describes the exception; and (iii) the class
Exception is supposed to be the root of the class hierarchy for "built-in, non-system-exiting exceptions" and user exceptions. You shouldn't raise Exception itself, but instead derive a specific exception class and raise an instance of that. So here you would need something like:class HTTPError(Exception): passAnd then:
raise HTTPError('Status code {}'.format(r.status_code))But in fact the
requests module already has a function for doing this, so all you need to do is:r.raise_for_status()-
Your interface for setting API parameters is to set them directly as properties of the API object:
api = APIClient()
api.userid = '123456'
api.key = '0tKk8fAyHTlUv'
api.base_url = 'https://api.com/'
api.test_url = 'https://test.api.com/'
api.testing = TrueIt would be better for the constructor to take these as keyword arguments: (i) this allows the constructor to check that all required parameters have been set; (ii) parameter values can be checked (if possible); and (iii) the keyword argument mechanism is more flexible than object properties since you can pass a sets of keywords around in the form of a dictionary. So I would expect to write:
api = APIClient(userid = '123456',
key = '0tKk8fAyHTlUv',
base_url = 'https://api.com/',
test_url = 'https://test.api.com/',
testing = True)Code Snippets
r = api.request(...) # might raise an exception
do_something_with(r)api.error = None # clear the old error, if any
r = api.request(...) # might set api.error
if api.error:
# handle the error somehow
else:
do_something_with(r)api.error = None # clear the old error, if any
r = api.request(...) # might set api.error
if api.error:
raise MyError("API error: {}".format(api.error))
else:
do_something_with(r)requests.exceptions.SSLError: hostname 'foo.com' doesn't match 'bar.com'raise ExceptionContext
StackExchange Code Review Q#33919, answer score: 9
Revisions (0)
No revisions yet.