HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Stack Exchange API Python library

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stackexchangepythonlibraryapi

Problem

I wrote a package a few years ago to make my interactions with the Stack Exchange API more straightforward. I've maintained it over the years and used it in several scripts I've written that interact with Stack Exchange. Now my goal is to release it to PyPI. Before I do so though, I'd like to ensure it's a clean, well written Python library. Ultimately, it'd be nice if another developer could look at the code and easily add features or fix bugs they encounter.

Due to my development environment, this is written for Python 2.7. However, I believe it is Python 3 compatible, but I do not have such an environment set up to actually test that.

The main library:

SEAPI.py

``
import requests
from itertools import chain
from time import sleep

try:
import json
except ImportError:
import simplejson as json

class SEAPIError(Exception):
"""
The Exception that is thrown when ever there is an API error.

This utilizes the values returned by the API and described
here: http://api.stackexchange.com/docs/types/error

Parameters
----------

url : string
The URL that was called and generated an error
error : int
The
error_id returned by the API (should be an int)
code : string
The
description returned by the API and is human friendly
message : string
The
error_name returned by the API
"""

def __init__(self, url, error, code, message):
self.url = url
self.error = error
self.code = code
self.message = message

class SEAPI(object):
def __init__(self, name=None, version="2.2", **kwargs):
"""
The object used to interact with the Stack Exchange API

Attributes
----------

name : string
(Required): A valid
api_site_parameter`
(avaiable from http://api.stackexchange.com/docs/sites) which will
be used to connect to a particular site on the Stack Exch

Solution

This is a very long __init__ function. Before looking any closer, I'd guess that some of this can be either abstracted away or reduced. A large function generally is bad, but particularly init should be bare bones to just do what's necessary to make an instance usable. If it does too much, then it's a red flag.

First off, you're setting default values, then checking if they're in the kwargs and then setting them again if they are. This is confusing and triples the lines you should have. Ternaries get a bad rap, but this is the perfect use case for them:

self.proxy = kwargs['proxy'] if kwargs.get('proxy') else None
self.max_pages = kwargs['max_pages'] if kwargs.get('max_pages') else 100


Though as @ferada and @jwodder point out, dict.get can return a default value. If you give it a second parameter it will return that if the key is not found, so even simpler is to do this:

self.proxy = kwargs.get('proxy')
self.max_pages = kwargs.get('max_pages', 100)


By default get returns None, so you can just use it plainly for proxy. With max_pages, passing 100 as the second parameter means that if the key isn't found, self.max_pages will be set to 100.

Now for the section where you try get the proper name and key. You set self._version again, for no particular reason as it has not changed. You also don't call break when finding the correct site, even though you only need to match one result and then __init__ is done.

I would also make your strings here constants, the filter and the base url you format with:

class SEAPI(object):
    BASE_URL = 'https://api.stackexchange.com/{}/'.format(version)
    FILTER = '!*L1*AY-85YllAr2)'


I think this pattern of doing too much extends to your other much more complicated functions. Ideally every function would have one job that it does in (relative) isolation. This makes code more readable, extendable and testable. For example fetch could have a lot of the opening chunk moved into a separate function called get_params that just returns a dictionary of parameters and use fetch just to fetch the actual results. It's even easier since you're using a class, that gets around a lot of the scope concerns that you can often have with adding functions.

This while pattern is bizarre:

while True:
        run_cnt += 1
        if run_cnt > self.max_pages:  # Prevents Infinate Loops
            break


Don't you achieve the same with:

while run_cnt <= self.max_pages:


and then adding to run_cnt at the end of the loop? Or just using for _ in range(self.max_pages)? I might be missing something, but if I am it should be clearly commented.

Code Snippets

self.proxy = kwargs['proxy'] if kwargs.get('proxy') else None
self.max_pages = kwargs['max_pages'] if kwargs.get('max_pages') else 100
self.proxy = kwargs.get('proxy')
self.max_pages = kwargs.get('max_pages', 100)
class SEAPI(object):
    BASE_URL = 'https://api.stackexchange.com/{}/'.format(version)
    FILTER = '!*L1*AY-85YllAr2)'
while True:
        run_cnt += 1
        if run_cnt > self.max_pages:  # Prevents Infinate Loops
            break
while run_cnt <= self.max_pages:

Context

StackExchange Code Review Q#120312, answer score: 3

Revisions (0)

No revisions yet.