iron / router

Router middleware for the Iron web framework.
165 stars 74 forks source link

URL generation #116

Closed gsquire closed 8 years ago

gsquire commented 8 years ago

An implementation for #109.

Some notes:

Please comment on design or anything that needs attention. I would appreciate your eyes @untitaker :)

leoyvens commented 8 years ago

The extra API complexity is unfortunate. Maybe we could use the glob itself as the identifier?

gsquire commented 8 years ago

I thought so, but couldn't you have two handlers share the same glob with a different action?

E.g.

router.get("/test", get_test);
router.post("/test", post_test);
untitaker commented 8 years ago

I completely missed this, giving it a look but unlikely to be able to test it out in the near future since I won't have access to a PC.

untitaker commented 8 years ago

Using the route pattern as identifier would defeat the purpose of this PR since you'd need to hardcode your patterns everywhere again.

We could make the explicit identifier optional for cases where you wouldn't care about URL generation (APIs?)

leoyvens commented 8 years ago

@gsquire The URL cares about the HTTP method?

@untitaker I see. Perhaps the glob could be the default id, so that we don't have to break the current API. The way I see optional parameters expressed in Rust is either by the builder pattern or by providing different methods with different names, i.e. named_route. We could even introduce syntax to encode the id in the glob.

untitaker commented 8 years ago

I agree and would prefer the builder pattern, by returning an object from the existing register method and its shortcuts. Macros are secondary and we should aim for a nice (i.e. non-verbose) API even if they are not used.

On 28 June 2016 17:09:35 EEST, Leonardo Yvens notifications@github.com wrote:

@gsquire The URL cares about the HTTP method?

@untitaker I see. Perhaps the glob could be the default id, so that we don't have to break the current API. The way I see optional parameters expressed in Rust is either by the builder pattern or by providing different methods with different names, i.e. named_route. We could even introduce syntax to encode the id in the glob.


You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/iron/router/pull/116#issuecomment-229060333

Sent from my Android device with K-9 Mail. Please excuse my brevity.

gsquire commented 8 years ago

@leodasvacas I was thinking about something else but realized it's not pertinent, so ignore me :)

So you would prefer using a builder pattern to add the parameters in? And just substitute each keyword with the next parameter? So the router type can then generate URLs without the auxiliary hash map I have now.

E.g.

router.get("/:version/:p").add_param("v1").add_param("test");
untitaker commented 8 years ago

Garret, that's an interesting API, I was more thinking about something like this:

router.get("/a/:b/").with_id("my_route")

And then have some equivalent to url_for, possibly requiring the builder pattern again.

Your variant might actually allow for stronger typing because it can verify at compile time that a route exists when generating a URL. On the other hand the user has to maintain a collection/struct of routes themselves (instead of having a hashmap provided by routing)

On 28 June 2016 19:14:22 EEST, Garrett Squire notifications@github.com wrote:

@leodasvacas I was thinking about something else but realized it's not pertinent, so ignore me :)

So you would prefer using a builder pattern to add the parameters in? And just substitute each keyword with the next parameter? So the router type can then generate URLs without the auxiliary hash map I have now.

E.g.

router.get("/:version/:p").add_param("v1").add_param("test");

You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/iron/router/pull/116#issuecomment-229099994

Sent from my Android device with K-9 Mail. Please excuse my brevity.

leoyvens commented 8 years ago

@untitaker What you wrote would be hard to do right now since get returns the router. How would we do this? Do we need a Route type which can be configured and then passed into router?

untitaker commented 8 years ago

You have to change the return type for either API.

On 3 July 2016 18:47:21 EEST, Leonardo Yvens notifications@github.com wrote:

@untitaker What you wrote would be hard to do right now since get returns the router. How would we do this? Do we need a Route type which can be configured and then passed into router?


You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/iron/router/pull/116#issuecomment-230159957

Sent from my Android device with K-9 Mail. Please excuse my brevity.

gsquire commented 8 years ago

If we went with this design:

router.get("/a/:b").with_id("my_route");

Then we could do this as well:

router.get(router.url_for("my_route").param("test"), handler);

However this would change the return type like you said. I'm quite perplexed now...

gsquire commented 8 years ago

This was a shot in the dark so I won't be offended if this should be closed. Feel free to do so or if you want we can work on finalizing the design.

untitaker commented 8 years ago

Sorry for the late response, but I've been busy. I'll open a new PR based on yours.