patternpythonMinor
Stack Exchange API Python library
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
``
(avaiable from http://api.stackexchange.com/docs/sites) which will
be used to connect to a particular site on the Stack Exch
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
First off, you're setting default values, then checking if they're in the
Though as @ferada and @jwodder point out,
By default
Now for the section where you try get the proper name and key. You set
I would also make your strings here constants, the filter and the base url you format with:
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
This
Don't you achieve the same with:
and then adding to
__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 100Though 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
breakDon'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 100self.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
breakwhile run_cnt <= self.max_pages:Context
StackExchange Code Review Q#120312, answer score: 3
Revisions (0)
No revisions yet.