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

Support matching parameters that containing the delimiter #811

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

6543
Copy link

@6543 6543 commented Apr 9, 2023

close #781

@6543 6543 marked this pull request as ready for review April 9, 2023 01:05
This was referenced Apr 11, 2023
tree_test.go Show resolved Hide resolved
@6543
Copy link
Author

6543 commented Apr 11, 2023

@pkieltyka @VojtechVitek please have a look!

PS: this might require a major version bump but I think it's worth it

Copy link
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

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

Thanks @6543. I think this looks great!


It'd be nice to provide a high-level description of the changes. Something like:

Given this route

r.Get("/files/{file}.{ext}", handleFileExt)

and a request to /files/photos.tar.gz path

chi will match the longest leftmost parameter first (same as Regex or filepath.Ext()):

fmt.Println(chi.URLParam(req, "file"))
fmt.Println(chi.URLParam(req, "ext"))

- photos
- tar.gz
+ photos.tar
+ gz

Yes, this can be considered a breaking change. But I'm not sure if it's worth the major version bump.

Anyway, let's see what @pkieltyka has to say

if segmentlen == -1 {
segmentlen = len(xsearch)
} else {
segmentlen += 1 // just keep the old code working
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how the "old code" was exactly :) Can you explain what this does (and why) in the comment?

@silverwind
Copy link

silverwind commented Apr 20, 2023

These failed cases do look bad imho. I think a option per-parameter option to catch a dot is needed. Maybe expose a constraints regex to the user. like rails does:

https://prathamesh.tech/2020/06/15/allowing-dots-in-rails-routes/

If that is not possible, maybe some special syntax like {*user_name}? Of course some users may also want to match across slash etc, so exposing a regex is best.

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.

Matching parameters containing the delimiter
3 participants