-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
Router improvements #179
Router improvements #179
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 83.63% 83.86% +0.23%
==========================================
Files 77 77
Lines 4277 4351 +74
==========================================
+ Hits 3577 3649 +72
- Misses 700 702 +2
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
1cf372a
to
0e08471
Compare
ee85223
to
b265d24
Compare
I've found an issue with the prefix / suffix capture. router.get(":name:/:ext:") { req in
req.parameters.get("name")! + "." + req.parameters.get("ext")!
} GET /file.jpg => 404 So we've got the following parameter cases:
Honestly the colon thing makes my head a bit dizzy to think about parameters. What if we use the following syntax, to make things a bit more unified:
I'd prefer [] characters instead of colons, it makes paths more readable. What do you think? Does this make any sense? 🤔 |
I assume you meant I agree on the double |
Yeah, so this won't work either: What if we keep the |
Also one more thing: router.get("**") { req -> String? in
print(req.parameters.getAll(":**:")) // [String]
print(req.parameters.getAll(
// won't work internal protection level
String(HBParameters.recursiveCaptureKey)
))
print(req.uri.path) // String
print(req.parameters.getCatchAll()) // String?
// req.parameters.getCatchAll() // [String]
return req.parameters.getCatchAll()
} The |
Anyway, I don't mind if we can't have multiple named captures within a component, I only needed catchAll, so this is already more than enough. Thanks for your hard work. 🙏 |
@tib I ended up using |
That's a very good idea! I'll take a look at the PR shortly and come back to you with my feedbacks. Many thanks again, this is going to be super useful. 🙏 |
Everything looks really great, one minor suggestion:
Could you return I suppose most of us will have to cast the |
You can do pretty much everything that |
Make sense, all right then. 👍 |
Capture paths caught by recursive wildcard Wildcards with both prefix and suffix
a379521
to
c9aa172
Compare
Capture paths caught by recursive wildcard
GET /test/this
will returntest/this
Wildcards with both prefix and suffix
Capture with both prefix and suffix. The parameter name is surrounded by ":" instead of just being prefixed with a ":"