Skip to content
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

Retain error details in string representation of errors generated by ApiTools. #266

Merged
merged 1 commit into from
May 7, 2019

Conversation

tvalentyn
Copy link
Contributor

Currently errors defined in ApiTools don't capture error message details in string representations of the errors:

from apitools.base.py.exceptions import HttpError
>>> HttpError("response", "content", "http://url")
HttpError()

This PR changes this behavior, for example:

>>> from apitools.base.py.exceptions import HttpError
>>> HttpError("response", "content", "http://url")
HttpError(u'HttpError accessing <http://url>: response: <response>, content <content>',)

Calling constructor of the superclass with one argument helps to capture the details of exception, as defined[1] in BaseException, the superclass of Exception, the superclass of apitools.base.py.exceptions.Error.

[1] https://github.com/python/cpython/blob/db7197543112954b0912e3d46e39fefcb1c3b950/Objects/exceptions.c#L111.

@coveralls
Copy link

coveralls commented May 6, 2019

Coverage Status

Coverage increased (+0.006%) to 91.681% when pulling cfb2304 on tvalentyn:retain_message_details into f3745a7 on google:master.

@tvalentyn
Copy link
Contributor Author

R: @houglum

@tvalentyn
Copy link
Contributor Author

tvalentyn commented May 6, 2019

cc: @chamikaramj , @aaltay.

@houglum houglum self-requested a review May 7, 2019 07:29
Copy link
Contributor

@houglum houglum left a comment

Choose a reason for hiding this comment

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

LGTM

Pulled the changes into a local clone of gsutil and ran integration tests (both XML and JSON) in Py2 and Py3 - everything passed, so this is fine from the gsutil end :)

@tvalentyn
Copy link
Contributor Author

@houglum @jameslynnwu @kevinli7
Could someone help merge this please?

@tvalentyn
Copy link
Contributor Author

Thanks for the review, @houglum .

@houglum houglum merged commit 7fd3df1 into google:master May 7, 2019
@tvalentyn
Copy link
Contributor Author

tvalentyn commented May 7, 2019

@houglum @jameslynnwu @kevinli7 Folks, do you know, when do we might have next apitools release? We are about to cut a new Beam release and I was wondering if we could upgrade to a new version of apitools that has this change. Thank you.

@tvalentyn tvalentyn deleted the retain_message_details branch May 7, 2019 20:11
@houglum
Copy link
Contributor

houglum commented May 7, 2019

@jameslynnwu @kevinli7 I can roll up a new release if neither of you have anything you'd like to get in before the next version.

@google google deleted a comment from jameslynnwu May 7, 2019
@houglum
Copy link
Contributor

houglum commented May 7, 2019

Done. 0.5.28 is out now @tvalentyn

@tvalentyn
Copy link
Contributor Author

Thanks a lot @houglum ! I really appreciate it!

catleeball pushed a commit to catleeball/apitools that referenced this pull request Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants