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

Simplify HTTP::Connection#read_headers! #456

Merged
merged 1 commit into from
Feb 12, 2018

Conversation

janko
Copy link
Member

@janko janko commented Feb 11, 2018

This simplifies the implementation of HTTP::Connection#read_headers!, the behaviour should be preserved.

@ixti
Copy link
Member

ixti commented Feb 12, 2018

I have a feeling that there was a reason why this part was written like that. read_more feeds data to the parser, so it should be performed before asking parser about anything.

@tarcieri tarcieri merged commit 145b2bc into httprb:master Feb 12, 2018
@tarcieri
Copy link
Member

Looks good, thanks!

@tarcieri
Copy link
Member

Oh whoops, just noticed @ixti's comment now. My bad.

@janko
Copy link
Member Author

janko commented Feb 12, 2018

@ixti From the functionality perspective it should be the same, as the @headers variable is defined only after all headers are received. If you'd find it more intuitive that @parser.headers? is called after the first read_more, I could swap

until @parser.headers?
  # ...
end

with

begin
 # ...
end until @parser.headers?

@ixti
Copy link
Member

ixti commented Feb 12, 2018

I think that's fine in fact. I might have mistook this with other place - and as we don't have specs for this it seems like more likely.

@ixti
Copy link
Member

ixti commented Feb 12, 2018

@janko-m I was referring a bit different thing. But let's keep what you have did. If we will face any issues - we will redo this :D

@janko janko deleted the simplify-http-connection-read_headers branch February 12, 2018 00:18
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