Skip to content

Handle multiple path variables at the same path level#38

Merged
matthewmcgarvey merged 1 commit intoluckyframework:masterfrom
matthewmcgarvey:multiple-path-variables
Oct 27, 2020
Merged

Handle multiple path variables at the same path level#38
matthewmcgarvey merged 1 commit intoluckyframework:masterfrom
matthewmcgarvey:multiple-path-variables

Conversation

@matthewmcgarvey
Copy link
Member

Fixes #1 (It called for raising an error, but it is easy enough to just handle it as expected)

Summary

This adds support for multiple path variables for a single section of a path. So, the router will now handle /users/:id and /users/:user_id/settings as expected. Before, it would only keep the first one that was added to the router and all others would be dropped. Now we store them all in an internal list.

Benchmark (with --release)

  • Before: 202 ms
  • After: 283 ms
  • Difference: -81 ms
  • Change: -40% (I always forget how to do the math so this might be wrong)

@jwoertink
Copy link
Member

Just so I'm clear here...

# this works currently
/users/:id
/users/:id/settings

# this does not, and is fixed by this PR
/users/:id
/users/:user_id/settings

@matthewmcgarvey
Copy link
Member Author

@jwoertink exactly

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@matthewmcgarvey matthewmcgarvey merged commit 6af06f6 into luckyframework:master Oct 27, 2020
@matthewmcgarvey matthewmcgarvey deleted the multiple-path-variables branch October 27, 2020 16:25
@paulcsmith
Copy link
Member

Nice!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Raise if there is a shared named parameter for a route

3 participants