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

[issue-2557] Add :from-:to range route formats #2560

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

Conversation

ProgrammingMuffin
Copy link

@ProgrammingMuffin ProgrammingMuffin commented Dec 17, 2023

Implements this feature (#2557)

Adds :from-:to route.

example /flights/:from-:to

image
image

@aldas
Copy link
Contributor

aldas commented Dec 17, 2023

This is little bit too simple take for implementing this feature. I would say that the requirement we are talking is allowing multiple parameters per route segment where segment is string part between / (slashes). Searching for param end to be / or - is little bit too restricted. Probably using syntax "/{month}-{day}-{year}" like Chi uses would be better.

p.s. using new special characters in route paths like { and } (or whatever character pair is selected) potentially breaks small number of users applications.

@ProgrammingMuffin
Copy link
Author

@aldas Thank you for your comment.

the / delimiter was already present and I don't think we should change that.

If I understood correctly, the restriction here is that users cannot use - in a parameter name like :user-name. Is that correct?

@aldas
Copy link
Contributor

aldas commented Dec 17, 2023

If I understood correctly, the restriction here is that users cannot use - in a parameter name like :user-name. Is that correct?

No, using only - is too restricting.

the / delimiter was already present and I don't think we should change that.

Sorry, I did not word this clearly. What I meant is that in addition to :param we could support {param} syntax and in that case we can have anything between two params like {file}.{ext}. As long there is something between that we can distinguish as separator.

router.go Outdated
@@ -660,7 +668,7 @@ func (r *Router) Find(method, path string, c Context) {
// act similarly to any node - consider all remaining search as match
i = l
} else {
for ; i < l && search[i] != '/' && search[i] != '-'; i++ {
for ; i < l && search[i] != '/' && !(search[i] == '-' || search[i] == '.'); i++ {
Copy link
Author

Choose a reason for hiding this comment

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

@aldas This is the best I could come up with, after spending some time trying around some stuff. Turns out that the dependency on "-" or "." is removed from the "route adding" and instead it is now on the "route detection". But one place should definitely have it or else, it cannot distinguish the params.

Copy link
Author

Choose a reason for hiding this comment

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

actually I might be wrong here. Let me go over this again

Edit done. This is resolved.

@ProgrammingMuffin
Copy link
Author

@aldas

image
image

@ProgrammingMuffin
Copy link
Author

Hi @aldas , I am just following up on this PR to see if there are any further changes to be made. Thank you for your time!

Copy link

codecov bot commented Dec 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (584cb85) 92.89% compared to head (d2b378c) 92.90%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2560      +/-   ##
==========================================
+ Coverage   92.89%   92.90%   +0.01%     
==========================================
  Files          39       39              
  Lines        4658     4668      +10     
==========================================
+ Hits         4327     4337      +10     
  Misses        240      240              
  Partials       91       91              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ProgrammingMuffin
Copy link
Author

@aldas Thank you for triggering the code coverage action. Looking forward to further updates on this PR.

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