Skip to content

Conversation

chhsiao90
Copy link
Member

Though there is useless for "reason" on the Response.
but I think keep it still better than discard when using reader to parse it.
So I create a PR to keep the reason on Response.

I had following modification:

  1. Retrieve reason when using reader to parse the incoming data by modify the regex
  2. Give reason default value empty string if the server don't care about it and will send empty string to client.

BTW, I know almost every client/server don't care about it,
but for my usage, my friend and I had implement a proxy server that was using h11,
and try keep original message that generated by client/server to prevent any misunderstand.
I had another modification on h11 that is related to it: keep original header field case.
I will create another PR after your feedback if you like it.

Thanks!

@codecov-io
Copy link

codecov-io commented Aug 26, 2016

Current coverage is 100% (diff: 100%)

Merging #13 into master will not change coverage

@@           master   #13   diff @@
===================================
  Files          20    20          
  Lines         867   867          
  Methods         0     0          
  Messages        0     0          
  Branches      169   169          
===================================
  Hits          867   867          
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update 0f0c468...65fff14

@njsmith
Copy link
Member

njsmith commented Aug 28, 2016

Meh, I'm not a fan of this part of HTTP, but I suppose you're right :-). Patch looks fine, but could you also update the docs to match?

Regarding header case: I am not sure that this is a good trade-off... it adds a lot of easy-to-mess-up complexity to user code if they have to worry about case normalization at use time. Note that nodejs's http stack unconditionally lowercases header names and they seem to manage OK.

(Also, as a heads up, I've been considering some much more radical changes to the header handling/representation, since the current code is really cumbersome in lots of situations.)

@chhsiao90
Copy link
Member Author

I will update the pydoc for this PR, actually I had planned to do this after your feedback!

For header part, I agreed that the solution is ugly, too. So I will re-design it.

Thanks for your feedback!

@chhsiao90
Copy link
Member Author

I had updated the doc for field reason on Response/InformationalResponse.
Please help check that the term I used is properly or is there any I could improved.
Thanks!

@njsmith
Copy link
Member

njsmith commented Oct 25, 2016

Looks good, thanks!

@njsmith njsmith merged commit 86e28bf into python-hyper:master Oct 25, 2016
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