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

Fix the documentation for wildcards for path definitions to match implementation #254

Merged
merged 1 commit into from
May 22, 2019

Conversation

gameldar
Copy link

Fixed the documentation for wildcards for path definitions for routing and added a bunch of tests that demonstrate the implemented behavior.

Description

The documentation referred to syntax like :name* but this isn't actually supported by the route recognizer, instead it is looking for *name for named path matches. So I've fixed the explanation and given more examples.

Additionally I added a number of tests for these different path options.

Motivation and Context

I was starting out with Tide and was trying to get some routes working - in particular capturing the rest of the path and so headed to the documentation to find the suggested wildcard of :path* which should match. Trying to use this caused issues with not being able to find the parameter back (as it turns out it is because the * gets added to the name so you can use context.param("path*") to get the value back - but it only matches the next segment).

Looking through blame I tracked it back to the underlying issue - #12 and I can see that isn't resolved/finalized yet. Looking at some of the behaviour that is possible (partly why the tests are there) - I can see that there might be some undesirable routing behaviour (e.g. just using : to namelessly match) might be an issue.

How Has This Been Tested?

I've run the tests after these changes and the extra tests I'ved added. I'm testing on Debian Testing using the nightly version of Rust from 2019-05-21.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

…lementation

Also add a bunch of tests that demonstrate the implemented behavior
@fairingrey
Copy link
Contributor

We ought to resolve #12 sometime in the future if and or when we acquire ownership of the route-recognizer crate or update it with something more flexible (and that we have control over, currently) like path-table. I'll mention it in that issue too, but wildcard behavior over dynamic paths/segments is something that's better resolved sooner than later, + taking another look at how we can design the router will also help with issues like #51.

Thanks for your contribution! I'll let one more person review it before a potential merge, but I like the changes.

@technetos
Copy link

There are a lot of use cases for tide and IIRC route-recognizer provides a mechanism for changing the priority of routes, though it may not be exposed publicly. My point is that there isnt a one size fits all route match priority and it would make sense to expose a mechanism in tide to change the priority so the most possible use cases can be supported.

@prasannavl
Copy link
Contributor

LGTM! Resolving the complete routing story will probably take a bit.
Thank you for the PR, @gameldar!

@fairingrey fairingrey merged commit 32709f5 into http-rs:master May 22, 2019
yoshuawuyts added a commit that referenced this pull request Nov 3, 2019
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
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

4 participants