-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
httpcaddyfile: Add shorthands for parameterized placeholders #3305
httpcaddyfile: Add shorthands for parameterized placeholders #3305
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was simpler than I thought it'd be!
For the record, it is also possible to use the existing list above for pretty much all these replacements, i.e.: {"query.", "{http.request.uri.query."
without using regexp, but as we discussed on Slack, performance doesn't really matter here, and with the regex we get the benefit of ensuring there's a closing curly brace }
rather than assuming. Probably not a big deal; if we wanted to simplify the code, that's one easy way to do it. (ref. this commit: 09075d8)
I would suggest that the regexp be compiled only once though, rather than in a loop.
16f4562
to
c44babd
Compare
@francislavoie When you have a chance could you help with the merge conflict? I'll do another review then. |
fdf9f9c
to
2aba6da
Compare
Rebased! Btw, easier to review with whitespace changes hidden. |
c3055d8
to
e236cf6
Compare
Thanks, looks good! My only remaining question is about the use of What do you think of |
|
e236cf6
to
ffc3b89
Compare
httpcaddyfile: Now with regexp instead httpcaddyfile: Allow dashes, gofmt httpcaddyfile: Compile regexp only once httpcaddyfile: Cleanup struct httpcaddyfile: Optimize the replacers, pull out of the loop httpcaddyfile: Add `{port}` shorthand
ffc3b89
to
1de9abf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, LGTM! Thanks Francis.
Adds the following new placeholder shorthands to the Caddyfile (plus one non-
*
placeholder):These are quite commonly used placeholders. Because they're relatively long, it's hard to remember the exact syntax on the fly, requiring frequent referencing of the docs to verify.
Currently I implemented this via just a quick prefix replacement, it's unlikely users would use these string patterns on their own. Probably the most concerning would be{r.
because it's such a short sequence.I chose the very short{r.*.*}
as the syntax for the regexp placeholder, because when users provide a name argument, it's pretty obvious whatr
is referencing, since the name will be associated with apath_regexp
orheader_regexp
matcher. It's easy to trace in the Caddyfile.Edit: Switched to
{re.*.*}
for some added clarity.As an improvement to this, in second iteration I think we could implement this via regexp replacements instead, it would probably be safer as well because we'd verify the final}
is actually present. Just wanted to get out a quick proof of concept first.Edit: Done, implemented using regexp. Pretty easy.