search

bmizerany / pat

MIT License
1.43k stars 115 forks source link

Modify URL.RawQuery only when necessary #4

Closed amattn closed 12 years ago

amattn commented 12 years ago

Currently, we are prepending the pattern variables (:name, etc) in all cases. Made a modification so this is only done when necessary.

bmizerany commented 12 years ago

Maybe I miss understand your comment. Are you prepending :name to avoid something?

amattn commented 12 years ago

Sorry about that.

:name was just an example. In the case of:

m.Get("/hello/:name", http.HandlerFunc(hello))

I call the ":name" part a pattern variable, I don't know if there is canonical terminology for it.

The changeset is attempting to address the problem in the following case

m.Get("/foo", http.HandlerFunc(foo))

There are no pattern variables. If I need to redirect /foo to something else, like /foo/, then the current code will unnecessarily prepend "&" in the RawQuery. If I only want to append a slash existing path, but keep the query and fragments, then I would get /foo?& after modifying and rebuilding the redirect url path.

I added a couple of tests to check for the edge cases I saw. I think if you run the tests without the changes mux.go it might be clearer.

Regardless, thanks for the nice little muxer.

bmizerany commented 12 years ago

Ah, that makes sense. When I get to my computer later today, I'll merge this. Thank you!

On Feb 22, 2012, at 7:17 AM, Matt Nunogawa @amattnreply@reply.github.com wrote:

Sorry about that.

:name was just an example. In the case of:

m.Get("/hello/:name", http.HandlerFunc(hello))

I call the ":name" part a pattern variable, I don't know if there is canonical terminology for it.

The changeset is attempting to address the problem in the following case

m.Get("/foo", http.HandlerFunc(foo))

There are no pattern variables. If I need to redirect /foo to something else, like /foo/, then the current code will unnecessarily prepend "&" in the RawQuery. If I only want to append a slash existing path, but keep the query and fragments, then I would get /foo?& after modifying and rebuilding the redirect url path.

I added a couple of tests to check for the edge cases I saw. I think if you run the tests without the changes mux.go it might be clearer.

Regardless, thanks for the nice little muxer.

Reply to this email directly or view it on GitHub: https://github.com/bmizerany/pat.go/pull/4#issuecomment-4105824

bmizerany commented 12 years ago

cherry-picked in. you'll need to update your master branch accordingly.