Skip to content

Bug: Catch any underlying exceptions from http.HTTPException #7

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

Merged
merged 3 commits into from
Dec 4, 2017

Conversation

Poogles
Copy link
Contributor

@Poogles Poogles commented Dec 1, 2017

Resolves #6

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@Poogles
Copy link
Contributor Author

Poogles commented Dec 1, 2017

Oops, commit name doesn't match mine. Will fix later.

@tswast tswast requested a review from theacodes December 1, 2017 20:33
@theacodes
Copy link

So, AuthorizedHttp.request shouldn't wrap exceptions at all. It should behave exactly like the thing it's wrapping. Conversely, Request should wrap exceptions. The difference here is that AuthorizedHttp is supposed to be a user-facing replacement for httplib2.Http and should more or less stick to the substitution principle, whereas Request is an internal facing construct that allows this library to be framework agnostic.

@Poogles Poogles force-pushed the master branch 2 times, most recently from 407cba8 to 1226ee4 Compare December 1, 2017 21:25
@Poogles
Copy link
Contributor Author

Poogles commented Dec 1, 2017

@jonparrott cheers for that. I saw it but seemingly ignored it in a rush to leave. That should now be fixed and I've changed the git user.name to one that should have signed the CLA (afaik).

@theacodes
Copy link

LGTM, but you'll need to please travis.

@Poogles Poogles force-pushed the master branch 2 times, most recently from 38a44d9 to eb9d13a Compare December 4, 2017 11:58
@Poogles
Copy link
Contributor Author

Poogles commented Dec 4, 2017

Travis now passes, you might want to check what I've done though. Rather than adding a unit test for an import error I've just added six to the requirements and fetched through there. Given that pretty much everything that targets 2/3 bundles six anyway it seems like the easiest thing?

CLA should pass now (how is it rerun?).

@theacodes theacodes merged commit e7cd722 into googleapis:master Dec 4, 2017
@theacodes
Copy link

Thank you, @Poogles!

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