- Notifications
You must be signed in to change notification settings - Fork1k
feat: add YAML support to server#6934
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ammario commentedApr 6, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@mafredri — this is probably 1k lines excluding generated code. I've manually tested that it works, but I'll write YAML docs and restore the support links docs in a separate PR with@bpmct to review. I also agree with making OptionSet a Edit: untagged you since you're on PTO. |
Uh oh!
There was an error while loading.Please reload this page.
cli/clibase/yaml.go Outdated
| comment:=wordwrap.WrapString( | ||
| fmt.Sprintf("%s (default: %s, type: %s)",opt.Description,defValue,opt.Value.Type()), | ||
| 80, | ||
| ) | ||
| nameNode:= yaml.Node{ | ||
| Kind:yaml.ScalarNode, | ||
| Value:opt.YAML, | ||
| HeadComment:wordwrap.WrapString(opt.Description,80), | ||
| HeadComment:wordwrap.WrapString(comment,80), |
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.
comment seems to be wordwrap'd twice
cli/clibase/yaml.go Outdated
| defValue="<unset>" | ||
| } | ||
| comment:=wordwrap.WrapString( | ||
| fmt.Sprintf("%s (default: %s, type: %s)",opt.Description,defValue,opt.Value.Type()), |
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 this would be nicer to read as%s\n(default: %s, type: %s) so default is always on the last line. Looking at the golden file it was hard to parse when the default/type randomly wrapped at 80 chars.
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.
Big fan of including the description as a comment, and the word wrapping makes it easy to read.
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 catch
| # An HTTP URL that is accessible by other replicas to relay DERP traffic. Required | ||
| # for high availability. | ||
| # (default: <unset>, type: url) | ||
| relayURL: |
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.
It seems like url types are the only fields that are printed out empty when the default is unset, the others seem to use the zero value.
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.
Maybe the type would be better asstring(url) so that it's clear you're inputting a string here? Unsure though.
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 for the most part it's self-explanatory.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This re-releases support links, amongst other benefits to operability.
Follow-up:
[]*Optionto avoid all the complex indirection seen in this PR