Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow capturing a postfix "rest of path" as route parameter #70

Closed
wants to merge 1 commit into from
Closed

Conversation

robinei
Copy link

@robinei robinei commented May 5, 2014

Allows the following syntax for route strings:
"/users/:userid/files/::path"

A ::-prefixed name may be supplied last in the string, and in the
example above this allows matching on strings like this:
"/users/123/files/Pictures/Summer2012"
where userid="123" and path="Pictures/Summer1012"
or
"/users/123/files/"
where userid="123" and path="".
(so unlike other parameters, this one can be empty)

Allows the following syntax for route strings:
   "/users/:userid/files/::path"

A ::-prefixed name may be supplied last in the string, and in the
example above this allows matching on strings like this:
   "/users/123/files/Pictures/Summer2012"
where userid="123" and path="Pictures/Summer1012"
or
   "/users/123/files/"
where userid="123" and path="".
(so unlike other parameters, this one can be empty)
@lhorie
Copy link
Member

lhorie commented May 7, 2014

This is a nice feature to have, but I have a few reservations:

  • double colon is not a super intuitive syntax (elsewhere double colon is usually the syntax for namespace resolution). It's also easy to create unintentional bugs due to typos, and it's easy to mistake it for a typo if one doesn't know what it is
  • the argument doesn't bind to the value correctly if used in a route like /foo/::id/bar

I added support for this and added a fix for the second point, but I'm proposing a slightly different syntax (/foo/:id..., /foo/:id.../bar) which is similar to variadic function syntax from C++, Java, etc. Documentation and tests have also been updated

The work above is done in origin/next and scheduled to be released in v0.1.12

@lhorie lhorie closed this May 7, 2014
@robinei
Copy link
Author

robinei commented May 7, 2014

This looks good :)

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.

None yet

2 participants