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

deps: Added methods to http parser #5601

Closed
wants to merge 1 commit into from

Conversation

RWOverdijk
Copy link

Affected core subsystem(s)

  • http

Description of change

As described in RFC2518, RFC4709, RFC5397, RFC5689 and RFC3744 there are a couple of request methods available that haven't been implemented yet.

I did find other issues discussing this, but until then I created a PR. I'm confident I made a mistake, as this is my first contribution to node, so give me hell :)

Running main() from gtest_main.cc
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from UtilTest
[ RUN      ] UtilTest.ListHead
[       OK ] UtilTest.ListHead (0 ms)
[----------] 1 test from UtilTest (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (0 ms total)
[  PASSED  ] 1 test.
/Applications/Xcode.app/Contents/Developer/usr/bin/make jslint
./node tools/eslint/bin/eslint.js benchmark lib src test tools/doc \
      tools/eslint-rules --rulesdir tools/eslint-rules
/Applications/Xcode.app/Contents/Developer/usr/bin/make cpplint
Total errors found: 0
/usr/local/opt/python/bin/python2.7 tools/test.py --mode=release message parallel sequential -J
[00:56|% 100|+ 1046|-   0]: Done

@joeherold
Copy link

👍

@mscdex
Copy link
Contributor

mscdex commented Mar 8, 2016

I think these changes should be submitted upstream in nodejs/http-parser first. Also, you'll probably want to include tests for the new methods.

@RWOverdijk
Copy link
Author

@mscdex I'll do that. I can take a look at the tests, but probably more importantly, I suppose these methods have to be implemented. The thing is, I don't really know where they're being implemented. Could you tell me where that is? I'll figure it out on my own, eventually. But some help could speed things up :)

@jasnell
Copy link
Member

jasnell commented Mar 8, 2016

Yeah, we won't be able to accept these changes here. They need to be opened against nodejs/http-parser

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.

4 participants