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

added optional groups to route syntax, with tests #1509

Merged
merged 2 commits into from Oct 3, 2012

Conversation

ianstormtaylor
Copy link
Contributor

from the ideas in #1508

@willium
Copy link

willium commented Aug 7, 2012

I'd love this to be merged, @documentcloud can we get this in?

@jashkenas
Copy link
Owner

Very nice. Can we use Rails' parentheses syntax instead of brackets for the optional parts? Make that change, and maybe add the little bit of docs about this to index.html, and I'll be glad to merge.

@ianstormtaylor
Copy link
Contributor Author

makes sense, i'll make that change very soon. thanks!

@ianstormtaylor
Copy link
Contributor Author

okay, switched to parentheses. added a section to the docs, and also broke up the route examples into the docs into separate paragraphs since it was getting very complicated to follow along after adding the third example.

@jashkenas
Copy link
Owner

Mind rebasing against master so there's a clean merge?

@ianstormtaylor
Copy link
Contributor Author

sure thing, all rebased and updated test names to match.

@jashkenas
Copy link
Owner

Thanks a bunch -- beautiful patch.

jashkenas added a commit that referenced this pull request Oct 3, 2012
added optional groups to route syntax, with tests
@jashkenas jashkenas merged commit 612496b into jashkenas:master Oct 3, 2012
@braddunbar
Copy link
Collaborator

Fantastic. This also allows optional trailing slashes, which I think will make a good many people happy.

routes: {
  "route(/)": "route"
}

@ultimatedelman
Copy link

this is phenomenal. thanks, @ianstormtaylor

@fredsterss
Copy link

@ianstormtaylor yeah boi!

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

Successfully merging this pull request may close these issues.

None yet

6 participants