Add public key address export subcommand #888
Conversation
NicolasMassart
added
enhancement
| * address in the file. A direct output of the address to standard out is not done because we | ||
| * don't want the address value to be polluted by other information like logs that are in | ||
| * KeyPairUtil that is inevitable. | ||
| */ |
There was a problem hiding this comment.
Suggestion: by default, print to stdout. That's pretty standard syntax for something like this. Let the user choose to add the file option if they want. I suspect a valuable use case is scripting (eg startup script) where user would rather grab from stdout (and run through grep if necessary). Dealing with files can be a PITA, depending on installation (permissions, stale file, stale file that doesn't get updated and thus user gets broken value)
There was a problem hiding this comment.
Yes but doesn't work as we output all logs to standard out. Files are way more easy to use just a cat and you got the content.
There was a problem hiding this comment.
See my comment in --to, if we make it optional we can default to std out. Do we have any CLI guidelines on positional parameters? That would be another option. Another option is to add another flag like --to-console and require one of those two flags.
However I think there should be some way to get it to output to console. The ease of use and default setting I am flexible on.
There was a problem hiding this comment.
agree on making the --to file optional
There was a problem hiding this comment.
--to is now optional. I also made the change on the other one. It's retro-compatible with the previous way to use the commands.
|
|
||
| @Option( | ||
| names = "--to", | ||
| required = true, |
There was a problem hiding this comment.
Suggestion: AFAIK: "--to" is not a standard name for output file. If you agree, consider using "-o / --out" instead as I think it is more common.
There was a problem hiding this comment.
but it's in our cli guidelines and how other commands works so we have to be consistant.
There was a problem hiding this comment.
--to is our CLI guideline.
I would recommend making it not required and in it's absence outputting to System.out.
There was a problem hiding this comment.
I'm in agreement with @shemnon. Using --to and if it's not specified output to the console.
There was a problem hiding this comment.
--to is now optional. I also made the change on the other one. It's retro-compatible with the previous way to use the commands.
NicolasMassart
requested review from
macfarla,
MadelineMurray,
shemnon and
jakehaugen
code, tests and doc changed
add public key address export subcommand make the file output optional and write to standard output by default code, tests and doc changes
PR description
Add
pantheon public-key export-address [--to=file]subcommand to write the node address to standard output or into a file.Also changes the --to option of public key export (the other subcommand) to behave the same way with optional --to option.
Fixed Issue(s)
fixes PAN-2319