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.

Sign up for GitHub

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

Jump to bottom

(feat) Allow trailing slash #84

Closed
wants to merge 1 commit into from
Closed

(feat) Allow trailing slash #84

wants to merge 1 commit into from

Conversation

leoyvens
Copy link
Contributor

@leoyvens leoyvens commented Jul 2, 2015

Partially fixes #82.

Urls with and without trailing slashes should be handled differently. A handler can exist with a trailing slash, and a different handler can exist without the trailing slash. Both should work independently.

Done.

An option, named something like RedirectTrailingSlash should exist. If it is set to true, and a requested route isn't found, but a route is found by adding (or removing) a trailing slash, a redirect is issued. This option should probably default to true.

The router now behaves as if such option was set to true. But the option is not implemented.

fn handle was refactored. Hopefully the result is clear and concise. These are my first lines of Rust, all feedback is appreciated!

@leoyvens
Copy link
Contributor Author

leoyvens commented Jul 3, 2015

Merged upstream changes (#85). Further refactoring. Is there some recommendation on commit squashing? Not used to rebasing things.

@reem
Copy link
Member

reem commented Jul 3, 2015

Looks great to me! Can you squash all the commits together to remove the merge commits? Otherwise this is ready to merge.

@leoyvens
Copy link
Contributor Author

leoyvens commented Jul 4, 2015

I've tried to squash the commits with all sorts of git rebase. But because of the merging and coflicts, it seems non-trivial to squash them, at least for me. I can create a new PR with a clean history, would that be preferable?

@reem
Copy link
Member

reem commented Jul 4, 2015

@leodasvacas the easiest way (if traditional git rebase -i is not working for some reason) is to just git reset HEAD~3 --soft then just recommit all the changes in a new commit, then push to this branch and pass --force to override the commits that have already been pushed which are now deleted. That should just work.

@leoyvens
Copy link
Contributor Author

leoyvens commented Jul 4, 2015

@reem Thanks for the assistance! I was avoiding that because I imagined it would generate a conflict on your end, but if that's fine, cool, it's done!

@reem
Copy link
Member

reem commented Jul 4, 2015

Looks like you were right, there is now a merge conflict :/

Since I don't want to block you on git stuff, I can just take it from here. Just for your information, what I'll do is pull this branch locally then run git pull --rebase upstream master where upstream is this repo. Then, when I fix conflicts it won't generate a merge commit.

@reem
Copy link
Member

reem commented Jul 4, 2015

Merged manually.

@reem reem closed this Jul 4, 2015
@reem reem mentioned this pull request Jul 4, 2015
Open
@leoyvens leoyvens mentioned this pull request Jul 10, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Not allowing a trailing slash is a bad idea
2 participants
@leoyvens @reem