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

Implementing extension methods in a way that does not slow down performance... #158

Closed
wants to merge 2 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 21, 2013

This does two things:

  1. If an extension method is detected, an optional callback is called
  2. All the characters in the method are validated using TOKEN(ch)

@ghost
Copy link

ghost commented Sep 29, 2013

+1 in the sense that I'm interested in this and would like to see it happen downstream in tmm1/http_parser.rb as well.

@dfa1
Copy link

dfa1 commented Oct 28, 2013

+1
I'm testing this patch, it works very well

@rlidwka
Copy link
Contributor

rlidwka commented Jan 8, 2014

+1

@indutny
Copy link
Member

indutny commented Jan 8, 2014

Generally, looks fine. Mind fixing some style issues?

@@ -12,3 +12,7 @@ parsertrace_g
*.Makefile
*.so.*
*.a

.cproject

This comment was marked as off-topic.

@indutny
Copy link
Member

indutny commented Jan 8, 2014

Also, please rebase it over the latest master.

@sintaxi
Copy link

sintaxi commented Jan 13, 2014

Anyone know of the status of this PR? I would love to see node get custom HTTP methods.

@indutny
Copy link
Member

indutny commented Jan 13, 2014

@sintaxi have you read my messages above? I think we could land it, but some fixes are required first.

@sintaxi
Copy link

sintaxi commented Jan 13, 2014

@indutny sorry, didn't mean to be dense. I read your messages but also see that the contributor has not been seen in this thread in 5 months and it looks like mostly formatting issues holding this back.

@jasnell are you still active on this PR?

@indutny
Copy link
Member

indutny commented Jan 13, 2014

@tjfontaine do you think it'd be fine if I'll land it with fixes myself?

@tjfontaine
Copy link
Contributor

@indutny if the user has signed the CLA you should be fine to land with your own fixes, if you change functionality you should maybe do it in a second commit though

@indutny
Copy link
Member

indutny commented Jan 13, 2014

I'm afraid that @jasnell didn't... @jasnell may I ask you to sign http://nodejs.org/cla.html ? It's the only real obstruction on the path of landing your PR.

@jasnell
Copy link
Member Author

jasnell commented Jan 13, 2014

Semi-active. I've been doing a few other things here lately. Can return to this later on this week if that's ok.

@indutny
Copy link
Member

indutny commented Jan 13, 2014

Sure, thanks for being at least semi-active! :)

@eliasgs
Copy link

eliasgs commented Mar 18, 2014

@jasnell any progress with this?

@chmac
Copy link

chmac commented Apr 7, 2014

@jasnell Were you able to sign the contributor agreement? Would be great to see this PR land...

@mk-pmb
Copy link

mk-pmb commented Apr 7, 2014

👍

@chmac
Copy link

chmac commented Jun 3, 2014

@jasnell Just a quick follow up, any chance you've already signed the Node contributor agreement? I'd love to see custom HTTP verbs supported in Node... :+1

@chmac
Copy link

chmac commented Jun 13, 2014

@indutny You still open to fixing the issues in order to merge this? With the recent removal of the CLA it seems like this is ready to move forward... 👍

@rlidwka rlidwka mentioned this pull request Jun 17, 2014
@indutny
Copy link
Member

indutny commented Jun 27, 2014

@chmac the problem with this patch is that we have very simple and short method detection in http-parser.

Basically, right now it thinks that COPY, CAPY, CQPY` is the same method. I don't think that there is much use in this implementation. Sorry.

However, here is a rebased and improved version of this patch: https://gist.github.com/indutny/3403029c5815fd0799ee .

@indutny indutny closed this Jun 27, 2014
@chmac
Copy link

chmac commented Jun 30, 2014

OK, I'm not sure about the implementation, I'm hoping to get custom http verbs in node... :-)

Is the gist ready to be merged? Does it need to be converted into a pull request?

@jamesamcl
Copy link

Basically, right now it thinks that COPY, CAPY, CQPY` is the same method.

Can I just say. That's pretty horrible.

@indutny
Copy link
Member

indutny commented Jun 30, 2014

@chmac the gist is finished, but please don't open a PR for it, because it is very unlikely that it'll be accepted.

@udp this is the price for speed, I guess. Though, I haven't benchmarked it.

@chmac
Copy link

chmac commented Jul 3, 2014

OK, so the gist is not a good solution, this PR is now not a good solution, is there another option?

Had something changed since this PR was ready to cleanup + merge? I spent a bit of time chasing a CLA, then that became unnecessary, but now we can't move forward with it. :-( Custom http verbs felt so close...

@indutny
Copy link
Member

indutny commented Jul 3, 2014

Heh, I feel your pain.

Well, perhaps we could introduce some limited support with the exceptions that I have mentioned. It'd be cool to hear @bnoordhuis opinion on it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.