-
Notifications
You must be signed in to change notification settings - Fork 58
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
proposal: use http.HandlerFunc instead of the 2 custom handler funcs #30
Comments
This is very interesting. I definitely like the idea and am really glad to see My main concern, of course, is with breaking backwards compatibility. Although a lot of other router packages are much more widely used, this package has been around longer than most of the Go vendoring solutions and is in at least one training course and a couple of commercial uses, and I would prefer not to inconvenience them if I can help it. That said, I don't think this is an insurmountable problem. The first thing that comes to mind is that on Go 1.7 there would be two ways to create the router. The object returned by the existing Then, some other constructor function would just create the new version of the router that uses native handlers and Contexts. I'm not sure exactly what this one would be called, but it would become the preferred way to create a router. I think it shouldn't be too hard to factor out the parts of the code that will require 1.7 into their own files so that it can build them conditionally, and it doesn't sound like any of the complex parts of the code are involved so the additional code maintenance overhead should be minimal. I'll have to think more about the best way to make that all happen, but I definitely would like to do this. Thanks for mentioning it! |
Great idea! |
Needs proper testing, which I'll do once Go 1.7 is released. Decided to just drop backward compatibility as the code was becoming complex. I will keep the originals style on master and publish this on a new branch when it's ready, and use gopkg.in to provide easy access to it. See #30
Needs proper testing, which I'll do once Go 1.7 is released. Decided to just drop backward compatibility as the code was becoming complex. I will keep the originals style on master and publish this on a new branch when it's ready, and use gopkg.in to provide easy access to it. See #30
I have something that passes tests on Go tip in the I also need to figure out a good way to publish both the original version and the Context-aware version of the software. I was hoping to use something like gopkg.in but it only works with tags and branches that look like version numbers, which isn't the end of the world but certainly isn't ideal. A fork may be another option though I'd prefer not to go that route if possible. |
Any plans to move forward with this? I completely agree about the preserving backwards compatibility. What about a second router type, which uses contexts? |
That's my hope, to just use two different types. Integrating them into the same code base without duplicating a ton of code proved a bit tricky but I'll have another go at it before long, work schedule permitting. |
Gotcha. If you don't mind, I'd be interested in taking a stab at what you've started in the |
That sounds great. Thanks! On Tue, Sep 6, 2016 at 11:58 AM, Patrick notifications@github.com wrote:
|
Fixed by #33 |
In go 1.7, the request has an extra context field. It can be used for storing the parameter map and the panic data, instead of using custom handler functions with an extra parameter.
I've played around with the idea in this fork:
https://github.com/urandom/httptreemux
To me it seems a good usage of the context is to store the whole parameter map as a single key. That's because usually when people have url parameters, they want all of them, as opposed just one or two items. The retrieval is also faster, since the context is essentially a linked list. The panic handler is quite similar.
I've added two functions in in the package,
RequestParams
andRequestError
, though I'm not sure if the names are perfect.If you approve of this proposal, perhaps it would be best if you make these changes in a version 5 branch, so you can still have version 4 if you want to break the api in some other way for the go1.6 and lower.
The text was updated successfully, but these errors were encountered: