Skip to content

Better error messages for (i)create_animations #628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 7, 2016
Merged

Better error messages for (i)create_animations #628

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b53e78
supported 5/9 of chris' suggested error msgs
Kully Dec 6, 2016
3993829
andrew's comments
Kully Dec 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions plotly/plotly/plotly.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1586,15 +1586,15 @@ def create_animations(figure, filename=None, sharing='public', auto_open=True):

api_url = _api_v2.api_url('plots')
r = requests.post(api_url, auth=auth, headers=headers, json=json)
r.raise_for_status()

try:
parsed_response = r.json()
except:
parsed_response = r.content

if 'error' in r and r['error'] != '':
raise exceptions.PlotlyError(r['error'])
# raise error message
if r.ok is False:
raise exceptions.PlotlyError(parsed_response['errors'][-1]['message'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would parsed_response not be a dict?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

504 gateway timeouts. An HTML response gets returned instead of JSON

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if the network is down. e.g. try turning off your wifi and then making the request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm good with this


if sharing == 'secret':
web_url = (parsed_response['file']['web_url'][:-1] +
Expand Down