surface NSIDC order and http download errors more effectively to user #195
Conversation
icepyx/core/granules.py
Outdated
| break | ||
| except KeyError: | ||
| if "errors" in results.keys(): | ||
| raise ValueError( |
There was a problem hiding this comment.
Not sure if API errors fit the usual interpretation of ValueError. Generally I think it is good to have more specific exceptions like "CMRQueryError" or something like that so that people using the library can handle those if they want to (maybe they are doing many searches for granules and sometimes it fails and they want to continue or retry later etc...), but that could be done later. Maybe RuntimeError instead, for now?
There was a problem hiding this comment.
That's a great point. I'm relatively new to raising errors, so haven't gotten beyond using built in ones yet. I've changed it to a RuntimeError for now, but I may see if I have a few minutes before the week is out to create a "CMRQueryError", as you suggest.
There was a problem hiding this comment.
@wallinb I added a QueryError class and tried to implement a custom error. I don't have any code to force a CMR error though - do you have an example I can use to test it?
There was a problem hiding this comment.
@JessicaS11 In general, I think sending bad input triggers an error response like this (400 status and 'errors' key in content) e.g.:
https://cmr.earthdata.nasa.gov/search/granules.json?version=003&temporal=badinput&short_name=ATL08
For writing tests, this or other error states should be mocked, e.g. can use a convenient library like responses
Now that I am looking closer at it, how about catching the error earlier with with response.raise_for_status()? Just adding that after the requests.get would result in an HTTPError exception thrown if there is a bad response status. If you want to do more, like raise a custom exception that prints the error message(s) CMR returns, then can do something like this:
try:
response.raise_for_status()
except HTTPError as e:
if 'errors' in response.content: # If CMR returns a bad status with extra information, display that
raise QueryError(response.content) from e # exception chaining will display original exception too
else: # If no 'errors' key, just reraise original exception
raise
There was a problem hiding this comment.
I used the responses library, as you suggested, to write a test, but I have no idea if it's doing what I think it is (and it seems more like it's testing the CMR API response than it would be testing the actual lines of code in granules.py... or am I missing something? I've read about using mocks for testing, but have never actually written one (until now?).
The primary purpose of this PR is to address the fact that CMR would return an error but the user would never see that particular error, which was making it difficult to diagnose issues. With your sample code, I was able to implement something that seems to be doing what we want. Feel free to make any adjustment/improvements!
Codecov Report
@@ Coverage Diff @@
## development #195 +/- ##
===============================================
- Coverage 50.50% 49.96% -0.55%
===============================================
Files 17 17
Lines 1281 1297 +16
Branches 280 284 +4
===============================================
+ Hits 647 648 +1
- Misses 581 596 +15
Partials 53 53
Continue to review full report at Codecov.
|
|
@wallinb I think this is finally ready to go. Thanks for all your help improving it! |
A few changes in response messages from NSIDC meant that users weren't be alerted when their subset orders did not contain any data in the region of interest (this used to return an error message).
May close #79.