Skip to content
This repository was archived by the owner on Nov 6, 2022. It is now read-only.

allow content-length and transfer-encoding: c… #518

Closed
wants to merge 8 commits into from

Conversation

veshij
Copy link
Contributor

@veshij veshij commented Jun 3, 2020

fixes #517

@veshij
Copy link
Contributor Author

veshij commented Jun 3, 2020

@indutny what would be the best way to pass new option to the parser? Use settings?

@veshij veshij changed the title WIP: allow content-length and transfer-encoding: c… allow content-length and transfer-encoding: c… Jun 5, 2020
@veshij
Copy link
Contributor Author

veshij commented Jun 5, 2020

Could anyone take a look at the change, please?

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, thanks for doing this.

I just opened #519 to add more Content-Length testing because that was rather lacking from the test suite... It would be good to land that one first and rebase this PR on top of it.

(You can rebase it now, of course, no need to wait for the actual merge.) #519 has been merged.

@bnoordhuis
Copy link
Member

bnoordhuis commented Jul 10, 2020

Thanks Oleg, landed with minor indent/line length touch-ups in d0f3b05 e13b274 (now with a PR-URL tag.)

@bnoordhuis bnoordhuis closed this Jul 10, 2020
bnoordhuis pushed a commit to bnoordhuis/http-parser that referenced this pull request Jul 10, 2020
Fixes: nodejs#517
PR-URL: nodejs#518
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Pierce Lopez <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants