Chung mesos http modified rebased #2
Conversation
The extension-pkg-whitelist that 'ujson' is added to contains a list of modules where C extensions may be loaded. Without being added to this list, pylint would barf on importing the 'ujson' package. In future commits we plan to use 'ujson' because it is significantly faster than the standard 'json' library because of these C extensions.
Previously, the requirements were duplicated across multiple files instead of pulled from a single source of truth.
Eventually we will move this virutalenv up a directory and have it include requirements from all python submodules.
| METHOD_POST = 'POST' | ||
| METHOD_PUT = 'PUT' | ||
| METHOD_PATCH = 'PATCH' | ||
| METHOD_DELETE = 'DELETE' |
There was a problem hiding this comment.
These act as symbolic constants for the methods, which are referred to by the method-specific methods like Resource.get() and Resource.post(). Why use strings instead of symbolic constants?
| return '/'.join([base_url.rstrip('/'), other_url.lstrip('/')]) | ||
| try: | ||
| url = '/'.join([base.rstrip('/'), other.lstrip('/')]) | ||
| except Exception as exception: |
There was a problem hiding this comment.
cf. https://docs.python.org/2/howto/doanddont.html#except
Catching all exceptions here essentially reduces the information content of any exceptions raised within the try block, blurring the distinction between code errors and runtime errors. Effective exception handling in Python usually consists of selectively catching runtime errors that may occur, and letting other unexpected exceptions bubble up so they can be caught and fixed.
Example: None was accidentally passed for the other param, but instead of getting an AttributeError and an informative stack trace that points all the way to the origin of the exception, we get MesosException instead with only 'NoneType' object has no attribute 'rstrip' in the message. This is a bit of a contrived example, but best practice for Python exceptions is to only handle exceptions that you know how to handle, and letting unexpected exceptions expose bugs that need to be fixed.
|
|
||
| def __init__(self, | ||
| url, | ||
| base_headers=BASE_HEADERS, |
There was a problem hiding this comment.
using a mutable objects as a default argument can cause unexpected behavior. cf. https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html
this is why None was passed in the original code instead of mutable objects like dicts.
|
|
||
| try: | ||
| response = requests.request(**kwargs) | ||
| except Exception as exception: |
There was a problem hiding this comment.
see above comment on catching all exceptions.
| use_gzip_encoding=use_gzip_encoding, | ||
| params=params, | ||
| **kwargs) | ||
| except Exception as exception: |
There was a problem hiding this comment.
see above comment on catching all exceptions.
| params=params, | ||
| json=payload, | ||
| **kwargs) | ||
| except Exception as exception: |
There was a problem hiding this comment.
see above comment on catching all exceptions
|
|
||
| try: | ||
| json = ujson.loads(response.text) | ||
| except Exception as exception: |
There was a problem hiding this comment.
see above comment on catching all exceptions
| raise HTTPException('Could not load JSON from "{data}": {error}' | ||
| .format(data=resp.text, error=str(exception))) | ||
| json = decoder(json) | ||
| except Exception as exception: |
There was a problem hiding this comment.
see above comment on catching all exceptions
Temporary PR for commenting on changes.