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
caddyfile: Support for raw token values, improve map
, expression
#4643
Conversation
7a548b8
to
41d1f56
Compare
41d1f56
to
bccb317
Compare
No way, Francis, you are a WIZARD. I look forward to reviewing this, hopefully tomorrow |
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.
Awesome, thanks. This is a neat improvement. Just a few nits/questions/ideas.
|
||
// ScalarVal gets value of the current token, converted to the | ||
// appropriate scalar type. If there is no token loaded, it returns nil. | ||
func (d *Dispenser) ScalarVal() interface{} { |
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.
Good name choice, but TypedVal()
also came to mind. Not sure which is better. I'm fine with this, but wanted to ask anyway to see what you thought.
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.
I think Typed would be better if we supported returning somekind of struct, but I think Scalar is clearer that only scalars will be returned (for now)
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.
Yeah, and that's what I was wondering too, if in the future we'd ever deserialize a string as a structured data type. But I can't see that happening in the near future nor do I have any idea why or what that would look like. So Scalar is fine... hopefully we don't have to change it.
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.
LGTM
Followup to 93c99f6 as pointed out in caddyserver/website#221, and fixing the issue mentioned in caddyserver/website#220
See the adapt tests, which cover how it behaves.
Note: This is a breaking change, because people using
map
with numbers used to get strings, and will now get numbers.