-
Notifications
You must be signed in to change notification settings - Fork 36
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.
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
Updated error message to include server side errors if present. #183
Conversation
src/kagglehub/exceptions.py
Outdated
@@ -58,6 +58,9 @@ def kaggle_api_raise_for_status(response: requests.Response, resource_handle: Op | |||
response.raise_for_status() | |||
except requests.HTTPError as e: | |||
message = str(e) | |||
server_error_message = response.json().get("message", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess some response are not json format so it fails some integration tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
src/kagglehub/exceptions.py
Outdated
if server_error_message: | ||
server_error_message = f"The server reported the following issues: {server_error_message}\n" | ||
server_error_message = "" | ||
if response.headers.get("Content-Type") == "application/json": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably do a case insensitive check.
add a try/except block to catch the requests.exception.JSONDecodeError
: https://requests.readthedocs.io/en/latest/user/quickstart/#json-response-content
This way, if the mime type is application/json
but the response is invalid, we won't break here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping the header check all together and just using a try/catch instead.
Error message to include server side error messages if present in payload.
BUG=b/375194356