tomaka / rouille

Web framework in Rust
Apache License 2.0
1.12k stars 106 forks source link

modify router macro #157

Closed jaemk closed 6 years ago

jaemk commented 6 years ago

issue #155

problems:

jaemk commented 6 years ago

Alright, so the router! macro should now support things like:

(GET) ("/float/5.0") => { ... },
(GET) ("/add/{a}/plus/{b}", a: u32, b: u32) => { ... },
(GET) ("/favicon.ico") => { ... },

Url parameters do have runtime parsing overhead, but normal urls should only do a string comparison.

If you wanted to also support intermixed ident urls, the router! macro would probably need to be broken up into a second macro that just matches a single route:

router!(request,
    rt!(GET, ("/number/5/{name}", name: String) => { ... }),
    rt!(GET, (/old/version/{num: u32}) => { ... }),
)
jaemk commented 6 years ago

Pretty sure I can replace the HashMap with a struct. I'll see what I can do.

tomaka commented 6 years ago

I don't want to break backwards compatibility. If you can accept both the old and new syntaxes then it would be good.

jaemk commented 6 years ago

Would it be acceptable if router! was "all-in" either old or new? Meaning you can't mix old and new style routes. So if you have a router with the current syntax it will continue to work, but if you want to use the new syntax you will have to update all your route definitions to the new style.

jaemk commented 6 years ago

So it seems like there's issues supporting both old and new macro patterns for rust versions prior to 1.20, the new pattern captures everything. I'm not sure why that is.

jaemk commented 6 years ago

We can get around the parsing problems (for rustc < 1.20) by changing the surrounding parens in the new router syntax to one of the following:

// option 1 (current)
(GET) ["/user/{id}", id: u32] => { ... },

// option 2
[GET] ("/user/{id}", id: u32) => { ... },

// option 3
[GET] ["/user/{id}", id: u32] => { ... },

// option 4
|GET| ("/user/{id}", id: u32) => { ... },

Have a preference? Option 1 is probably the least obstructive to change to since you'd need to update that section anyway if you were migrating.

jaemk commented 6 years ago

Pinging @tomaka , can you review this latest version? router! will accept both old and new syntaxes so this shouldn't break any existing usage. If you choose to use the new syntax, all url definitions will need to be updated (can't mix old and new syntaxes in the same router! invocation)

// new
(GET) ["/user/{id}", id: u32] => { ... },

// old
(GET) (/usr/{id: u32}) => { ... },
tomaka commented 6 years ago

I'm ok with the change, but I don't think it should replace the existing style yet. In other words I don't think we should change the examples just yet.

jaemk commented 6 years ago

Alright, I reverted the docs and examples to the existing style

tomaka commented 6 years ago

Looks good, thanks!