Handle Premium Only Errors #62
Conversation
Also add additional builds to Travis.
theaeolianmachine
force-pushed
the
handle-402
branch
from
49611cb to
2ab93f9
Compare
joetrollo
left a comment
joetrollo
left a comment
There was a problem hiding this comment.
Sorry, I'm super opinionated about Python.
asana/client.py
Outdated
| # TODO: Change back to default behavior once 1.0 & 1.1 have | ||
| # the same premium only response | ||
| asana_error = STATUS_MAP[response.status_code](response) | ||
| premium_only_str = "not available for free" |
There was a problem hiding this comment.
Constants in Python should be defined as UPPER_SNAKE_CASE and also be defined as class- or module-level variables rather than being instantiated with every call. I would move this to right below ALL_OPTIONS, and would also prefix it with an underscore to signify that this only has meaning for the internal implementation: _PREMIUM_ONLY_STR.
There was a problem hiding this comment.
Makes sense, +1.
asana/client.py
Outdated
| asana_error = STATUS_MAP[response.status_code](response) | ||
| premium_only_str = "not available for free" | ||
| if isinstance(asana_error, error.ForbiddenError) and ( | ||
| premium_only_str in str(asana_error)): |
There was a problem hiding this comment.
The AsanaError class doesn't define a __str__ magic method, so str(asana_error) is dubious. Prefer instead asana_error.args[0] which is a defined attribute inherited from Python's Exception. Alternatively, edit the base class AsanaError to save the message it builds as an instance attribute like self.message.
There was a problem hiding this comment.
Yeah I originally did exception.msg but it's not compatible with Python 3, and went off of the docs for BaseException. That being said for clarity, I'll just go with the route of storing the message as a part of the exception.
| raise error.PremiumOnlyError(response) | ||
| raise asana_error | ||
| elif response.status_code >= 500 and ( | ||
| response.status_code < 600): |
There was a problem hiding this comment.
This formatting is peculiar, and I think moving the opening paren to right after the elif would look cleaner.
There was a problem hiding this comment.
Yeah I've always had trouble figuring out the way that looks best for me for breaking up a if statement that's too long - looking over PEP8 again though, I think I'll go with what you're saying, which I think is this (https://www.python.org/dev/peps/pep-0008/#indentation)
if (this_is_one_thing
and that_is_another_thing):
do_something()
|
As am I, and I appreciate the feedback! |
Support 402 & 403 for premium only.