Better error messages for (i)create_animations #628
Conversation
| raise exceptions.PlotlyError(r['error']) | ||
| # raise error message | ||
| if r.ok is False: | ||
| raise exceptions.PlotlyError(parsed_response['errors'][-1]['message']) |
There was a problem hiding this comment.
Hmm, I don't understand why we can assume that ['errors'][-1]['message'] will exist? Certainly a 500 Internal Server Error won't have that, right?
There was a problem hiding this comment.
Won't this just result in a KeyError that will confound and frustrate users? BEFORE, at least we got a HEY, you got a 404! or something from r.raise_for_status().
There was a problem hiding this comment.
Can we test this out? How do I test a 500 Internal Server Error. And the whole point of me adding the message here is that the 404 or whatever was not elucidating for people: a message like A file with that filename already exists is more practical for a Plotly user
There was a problem hiding this comment.
Can we just do something like:
if not r.ok:
message = ''
if isinstance(parsed_response, dict):
errors = parsed_respnonse.get('errors')
if errors and errors[-1].get('message'):
message = errors[-1]['message']
if message:
raise exceptions.PlotlyError(message)
else:
# shucks, we're stuck with a generic error...
r.raise_for_status()
There was a problem hiding this comment.
When would parsed_response not be a dict?
There was a problem hiding this comment.
504 gateway timeouts. An HTML response gets returned instead of JSON
There was a problem hiding this comment.
Also if the network is down. e.g. try turning off your wifi and then making the request
There was a problem hiding this comment.
Yeah, that worked last time I tried in here...
Should we check if the figure is malformed though?
I tried passing a figure by changing the label data to datas (obviously wrong) and the request went through with all but a title and no plot.
There was a problem hiding this comment.
Nope, we can assume that if the request is OK, the content should be predictable. Our api is supposed to remain backwards-compatible and so this shouldn't be a problem.
There was a problem hiding this comment.
Okay, I'm good with this
|
@theengineear good to go? 💃 ? |
|
💃 ! |
Regarding Chris' suggestion here: #584 (comment)
Haven't tested all of the ways in which a request can fail, mostly because most of the
not doneones I don't know how to test (i.e.Request was Throttled,Server was Down) Yes, I said most when we're talking about 2 of the 3 on the list. 🙂Figure was malformed