Skip to content

Conversation

@bsipocz
Copy link
Member

@bsipocz bsipocz commented Feb 25, 2015

Would it make sense to check the response status for a query? If a client/server error is returned, it may be useful to raise an exception rather than return it as a valid result.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can just be response.raise_for_status(), you don't need the if statement or anything. At least, I thought that's what raise_for_status was supposed to do (raise an exception if status is bad, else pass)

Copy link
Member Author

Choose a reason for hiding this comment

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

@keflavich - indeed, it works like that. Thanks.

@keflavich
Copy link
Contributor

Yes, I think this is a good idea.

@bsipocz
Copy link
Member Author

bsipocz commented Feb 25, 2015

@keflavich - Addresed your comment. Does it need a changlog entry?

@keflavich
Copy link
Contributor

Sure, call it a bugfix: this should have been the behavior all along.

@keflavich
Copy link
Contributor

It looks like the MockResponse objects need a raise_for_status method too

@bsipocz
Copy link
Member Author

bsipocz commented Feb 26, 2015

Fixed. It's a wonder how it passed locally before I opened the PR.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 66.62% when pulling eeaf478 on bsipocz:utils_check_response_status_code into ac827c9 on astropy:master.

keflavich added a commit that referenced this pull request Feb 26, 2015
Check response status code and raise if it is an error
@keflavich keflavich merged commit 28e2d19 into astropy:master Feb 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants