-
Notifications
You must be signed in to change notification settings - Fork 20.2k
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
geth/dumpconfig: fix undocumented helptext #26658
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.
Minor nitpick, otherwise LGTM
cmd/geth/config.go
Outdated
Usage: "Show configuration values", | ||
ArgsUsage: "", | ||
Usage: "Export configuration values in a TOML format", | ||
ArgsUsage: "[dumpfile]", |
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.
ArgsUsage: "[dumpfile]", | |
ArgsUsage: "<dumpfile (optional)>", |
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.
No need to use <angle brackets(optional)> here since square brackets indicate this arg is optional.
If we use angle brackets for optional args like this, why do we need square brackets?
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.
since square brackets indicate this arg is optional.
Oh, that is something I did not know. I don't think we've used that way of writing ArgsUsage in other places in go-etherum
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.
Realized that go-ethereum is using mixed convention...
IMHO, it doesn't seem like a big deal (not confusing), so I will fix my PR if you still prefer angle brackets here.
go-ethereum/cmd/geth/chaincmd.go
Line 109 in 241cf62
ArgsUsage: "<filename> [<blockNumFirst> <blockNumLast>]", |
go-ethereum/cmd/p2psim/main.go
Line 189 in 241cf62
ArgsUsage: "<node> <method> [<args>]", |
go-ethereum/cmd/geth/misccmd.go
Line 85 in 241cf62
ArgsUsage: "<versionstring (optional)>", |
Line 87 in 241cf62
ArgsUsage: "<start (optional)>", |
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 a lot of users, like me, are not aware of the difference between [foo]
and <foo>
, so for clarity's sake let's spell it out: <dumpfile (optonal)>
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.
Could you review #26729 for this?
Github closed this PR automatically when I sync go-ethereum
to the HEAD...
This patch documentizes the 'saving into a file' option.
As suggested in #16383, piping dumpconfig's stdout is imcompatible in the Windows powershell due to the UTF16LE encoding.
To fix this, #18327 (428eabe) added an option to save dumpconfig in a file.
However, users never notify this feature (saving into a file) without searching the relevant github issue or seeing source code.
Thus, this patch fixes the aforementioned inconvenience by improving the description of the dumpconfig's help text.
fixes: 428eabe
Signed-off-by: Sungwoo Kim git@sung-woo.kim