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

Fixes #937 - Adds HTTP method and URL to exceptions. #961

Merged
merged 3 commits into from
Jul 3, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Jul 2, 2015

@jgeewax Taking #938 over since it lingered.

@tseaver I rebased and will shortly send another commit addressing review feedback.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Jul 2, 2015
Addressing comments in review and tox failures.
@dhermes
Copy link
Contributor Author

dhermes commented Jul 2, 2015

@tseaver PTAL

The things left unaddressed from the last PR were as follows:


From @tseaver @jgeewax: Wasn't sure about a couple things, so wanted to ask...

  1. Name of the variable (request_string). Seems expressive, but would rather pass a request object or something similar. Should I rename? Or pass two (method and url) on to make_exception?
  2. Assembling the request string in the make_exception call (seemed like it might be better done on a separate line?)

From @dhermes: Should we have a newline somewhere in the error message if it gets too long? (I lean towards no because of line wrapping.)

@tseaver
Copy link
Contributor

tseaver commented Jul 2, 2015

request_string seems wrong -- how about description or info?

I'm -0 on injecting a newline (thinking e.g. about logfiles: human readability vs. parsing by machine).

@dhermes
Copy link
Contributor Author

dhermes commented Jul 2, 2015

WDYT of error_info or extra_error_info?

@tseaver
Copy link
Contributor

tseaver commented Jul 3, 2015

error_info SGTM.

I've closed #938

@dhermes
Copy link
Contributor Author

dhermes commented Jul 3, 2015

@tseaver Are we all good then?

@tseaver
Copy link
Contributor

tseaver commented Jul 3, 2015

LGTM

@jgeewax
Copy link
Contributor

jgeewax commented Jul 3, 2015

LGTM - sorry for letting this linger. Thanks for cleaning it up for me :(

@dhermes
Copy link
Contributor Author

dhermes commented Jul 3, 2015

No worries @jgeewax

dhermes added a commit that referenced this pull request Jul 3, 2015
Fixes #937 - Adds HTTP method and URL to exceptions.
@dhermes dhermes merged commit af97ff1 into googleapis:master Jul 3, 2015
@dhermes dhermes deleted the jgeewax-937-more-helpful-errors branch July 3, 2015 15:32
@dhermes dhermes mentioned this pull request Jul 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no This human has *not* signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants