URL generation v2

[untitaker]

Fix #109 Continuing #116

cc @gsquire

I realized that the whole API ideas a-la

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

come with severe ownership problems, as we would have to return an object that would have to hold a pointer back to the router struct. I'm not sure if that's worth it.

[gsquire]

Thanks for salvaging this. I should have thought through my design more before submitting a PR.

As mentioned before, it's a bummer that this breaks the current router! macro API. If this is OK, I would be happy to submit some unit tests for this feature.

[untitaker]

The API design is ok and the same as in my PR, but the function for generating URLs can be done in less code.

On 15 August 2016 18:27:19 CEST, Garrett Squire notifications@github.com wrote:

Thanks for salvaging this. I should have thought through my design more before submitting a PR.

As mentioned before, it's a bummer that this breaks the current router! macro API. If this is OK, I would be happy to submit some unit tests for this feature.

You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/iron/router/pull/124#issuecomment-239851397

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

[untitaker]

@reem any feedback?

[gsquire]

Alright, well I am always happy to help get this through and will follow this thread.

[untitaker]

@reem Any comments? I'd like to see this API in the next release as-is, but it's an API breakage. Nevermind, found a few things.

[leoyvens]

Nice work with the macro. The new API is a bit brittle but I find it preferable to a breaking change.

[untitaker]

There is IMO no way to make this (EDIT: the macro) fully backwards compatible due to syntax restrictions. In particular, the handler must be a valid token tree, which requires e.g. parenthesis around closures and most expressions.

[untitaker]

Also this appears not to work with Rust stable at the moment. I don't really know why, the error messages make no sense ("Expected expression, found...", followed by an expression).

[untitaker]

This might actually be mergeable if we wait for the next major Rust version.

[gsquire]

Would you be opposed to leaving route ids out of the existing macro? If instead you made another macro like you did for the route_id function, that would make the API more consistent. This is just my opinion of course.

Nice job on your approach to not breaking the existing functions.

[untitaker]

Would this macro be a replacement for the current router! macro (e.g. router2!)?

[gsquire]

I was actually thinking of having something like this:

router!(get, "/" => handler);
router!(post, "/:q" => poster);
route_id!("test");

However that would limit users to adding a route id to the last route they list in the macro.

So I thought some more and was wondering what you would think of the following. Using the existing router API as is (macros included) and then adding the route id functionality as such:

Maintaining a set of all registered patterns and then having the route_id function take a pattern and an id.

router.get("/", handler);
router.route_id("/", "index");

You could perform a lookup in the set to see if it has been registered, if so, add it to the route id map. It would be extra bookkeeping but it would allow for an easier API I believe. You could add as many handlers as you want and then register route ids in any order.

[untitaker]

Invoking multiple macros is not possible since one macro invocation is creating a new Router. One would have to pass an existing router to the macro to add routes, but then the macro doesn't save any syntax at all.

What I want to do with the router! macro is to automatically register route IDs for invocations like this:

router!{
    get "/" => index_handler
}

With the PR as-is, this invokes stringify!(index_handler), which results in "index_handler", which can then be implicitly used as route ID for the just registered route. This is what Flask is already doing.

What I'm also a bit concerned about is separating the routes too much from the function definition. Ideally I'd like to have the definition of method, glob, route_id and the function def as closely together as possible. I.e., I don't like code like this:

let router = Router::new();

fn a(...) -> ... { ... }
fn b(...) -> ... { ... }
fn c(...) -> ... { ... }

router.get("/a", a);
router.get("/b", b);
router.get("/c", c);

router.route_id("/a", "a");
router.route_id("/b", "b");
router.route_id("/c", "c");

And generally prefer code like this:

let router = Router::new();

router.get("/a", |...| {
    ...
}).route_id("a");

router.get("/b", |...| {
    ...
}).route_id("b");

router.get("/c", |...| {
    ...
}).route_id("c");

Perhaps we shouldn't touch the current macro at all, and experiment with advanced macros in separate crates.

[gsquire]

Ah I see, missed that completely.

That's totally fair on having too much separation, I was thinking too simplistically. The macro could be changed later, sure. After all it is a syntactic sugar for the functions.

Thanks for discussing this!

[reem]

I think it might be better to just release a breaking edition which has a second set of methods which take an id (or a single set of functions with an optional id parameter) and a macro that just takes an id for every route. I worry this API is too much magic.

[untitaker]

Like this? I guess it's less magic..

[untitaker]

Also we need to come up with a way to attach url_for to request.extensions.

[untitaker]

Done! I think this PR is now ready for review again.

[reem]

So another alternative is to make the id non-optional, what do people think about that?

[untitaker]

Makes sense.

[gsquire]

Thanks @untitaker for the diligent work on making this what it is. This is why I keep wanting to contribute to iron.

[untitaker]

Thanks, this means a lot to me!

On Fri, Aug 26, 2016 at 02:22:23PM -0700, Garrett Squire wrote:

Thanks @untitaker for the diligent work on making this what it is. This is why I keep wanting to contribute to iron.

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

[leoyvens]

What about having an empty string mean the route has no id?

[untitaker]

I feel like this pattern is more common in interpreted languages, and a complete antipattern in Rust for some reason.

On 27 August 2016 00:05:14 CEST, Leonardo Yvens notifications@github.com wrote:

What about making an empty string mean the route has no id?

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

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

[leoyvens]

Lazy people may not want to give ids to routes, and just enter "" on all routes and be surprised by a panic.

[Hoverbear]

@leodasvacas Rust is not a language for lazy people... But you do make a good point. I wonder if we can push this error to compile time somehow?

[untitaker]

Afaict empty strings work fine as route_id, the error is when using the id twice. I don't think it's possible to detect that at compile time without syntax extensions.

On 27 August 2016 12:38:32 CEST, Andrew Hobden notifications@github.com wrote:

@leodasvacas Rust is not a language for lazy people... But you do make a good point. I wonder if we can push this error to compile time somehow?

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

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

[untitaker]

I'd like to merge as-is. @reem is this ok?

[reem]

So this looks pretty good to me, but I would like to pitch one more alternative for discussion before we settle on this approach.

What if we removed chaining from the API and had the route-adding methods return a Route object, and you'd do something like this:

struct App<H> {
     handler: H,
     routes: Routes
}

struct Routes {
    hello: Route,
    goodbye: Route
}

fn hello(..) { .. }
fn goodbye(..) { .. }

fn make_app() -> App {
    let mut router = Router::new();
    let routes = Routes {
        hello: router.get('/hello', hello),
        goodbye: router.get('/goodbye', goodbye)
    };

    App { handler: router, routes: routes }
}

then you'd call make_app in main and in your tests.

Router's url-generation methods would take the Route object, which could just contain a sequentially or randomly assigned id. This also allows Route to not have to borrow Router, which would ruin the API.

This makes the usage of the API slightly more involved, but adds structure and moves nearly all errors to compile time. (It does open up one more source of error, using a Route with a Router other than the one it was created with. It is possible to fix this at compile time as well but it's probably not worth the ergonomics hit.)

What does everyone think?

[untitaker]

I do consider your solution to be "more pure", I opted for the current one because it already does all the state management for you. Your solution comes with the extra boilerplate in form of the Routes struct one has to write themselves. I feel like we could eliminate that too using macros, but then I'm not really sure if that's worth the extra API complexity (and the mandatory use of macros).

[reem]

It's true, that's why I'm still on the fence about this design. Your suggestion of returning a generated Routes struct from the router macro is a good one. Maybe lets get another opinion before we decide? On Sun, Aug 28, 2016 at 1:25 PM Markus Unterwaditzer < notifications@github.com> wrote:

I do consider your solution to be "more pure", I opted for the current one because it already does all the state management for you. Your solution comes with the extra boilerplate in form of the Routes struct one has to write themselves. I feel like we could eliminate that too using macros, but then I'm not really sure if that's worth the extra API complexity.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/iron/router/pull/124#issuecomment-242997124, or mute the thread https://github.com/notifications/unsubscribe-auth/AEhn3SiZrEca3_cx2NYLt3tki2INJ9_9ks5qke6lgaJpZM4Jj_hp .

[untitaker]

Also I'd like to add url_for to templating languages. The current options (handlebars in particular) are very weakly typed, I'm not sure if an API that takes advantage of all Rust has to offer is harder to embed that way.

[gsquire]

My 2 cents:

I would like to use functions that an API provides and avoid using my own types even if they're strictly defined in documentation. Granted you could make functions to return a new App or a new Routes.

At the end of the day I want the "easy" path when using third-party code.

[untitaker]

Unless somebody comes up with a PoC for the API @reem proposed, I'd rather merge this one sooner than later and just see how it is recieved by the community.

In general I do prefer more strongly typed APIs (in particular rust-url and std::path are superior to the Python counterparts I am used to), but I don't see much value in having one here for two reasons:

• the current state of templating engines: https://github.com/Keats/tera/issues/24 is a good discussion about this. Basically you can only pass JSON into templates. You can circumvent this by passing pre-generated URLs into the render context, and to be fair, template engines are currently such a PITA that this one doesn't matter that much.
• If you want to use that API without having a shitload of boilerplate, you're forced to use macros which make code hard to reason about, and inevitably lead to ugly error messages unless the macro is trivial (probably won't be the case) or you're using syntax extensions (not in stable Rust yet).

[reem]

Ok, I agree with @untitaker. Let's get this branch out there and available for people to use and see how it is adopted. We shouldn't be too afraid of releasing breaking changes to experiment at this point - the ecosystem is not that stable.

It's possible to create a dynamic API based on the static one I proposed earlier (just make a HashMap<String, Route> instead of a new type) so I think it's something we should consider next.

[untitaker]

\o/

[reem]

@untitaker I left one more comment if you want to address it before releasing.

[untitaker]

Sorry for the quick merge. I've responded to your comment. Anything else to be done before release?

[reem]

Don't think so. It's all good!

[untitaker]

0.3.0 is out.