Refactor commands to use the go-flags package properly. #71
Conversation
Commands and subcommands can now be used in a simpler way.
|
Some observations from preliminary testing:
I think 1 primary trigger for this refactor is to enable us use command options, can you provide an example to that effect? Use the receive command, let's assume it accepts an "AccountNumber" option, type uint32, short "n", long "account". This option is only useful when executing the receive command. I'm eager to see how this plays out Once we have those issues ironed out, I'll take a deeper dive into the code |
Simplify output of running ./dcrcli without a command
Use upstream go-flags dependency.
Commands and subcommands can now be used in a simpler way.
Use upstream go-flags dependency.
Can you check those again? The behaviour has changed.
Oops, I used the // file: cli/commands.go
type SendCommand struct {
Custom CustomSend `command:"custom"`
}
type CustomSend struct {
Address string `short:"a" long:"address"`
}
func (c CustomSend) Execute(args []string) error {
fmt.Println("Custom send called")
fmt.Println("address value:", c.Address)
return nil
}We register the command as follows. // file: cli/app.go
type AppCommands struct {
Send SendCommand `command:"send" description:"send a transaction" subcommands-optional:"optional"`
// other fields
}
var DcrcliCommands AppCommandsAnd we parse it as follows. // file: main.go
cli.Setup(client)
parser := flags.NewParser(&cli.DcrcliCommands, flags.Default)
if _, err := parser.Parse(); err != nil {
// handle error
}Below is help output when running the compiled code. When we run the When we run the If we use the flag without providing an argument, the following error is shown. That's mostly it. |
There was a problem hiding this comment.
Feedback from testing, haven't really paid another attention to the code. I believe this PR comes with changes to the behaviour of running dcrcli (i.e. without commands), dcrcli -h and dcrcli <command> -h. Those were scenarios I tested for in getting the feedback below.
A fourth use case I simulated in my head but I know this PR also introduces changes regarding, is command-specific options. According to my simulation, this should work well (for the most part). The exception, a pointed below, is when using a command-specific option when running in cli mode. Since commands are limited to cli mode, command-specific options should throw an error when parsing if the user is running on http mode.
I believe the the identified issues below are fixable. An adjusted version of this PR demonstrates potential ways of fixing some of the identified issues. Reference links are provided in such cases.
|
We'll need to update the Using dcrcli section of the readme |
|
For reference purposes, here are the objectives of (or issues affected by) this PR:
PS: I haven't reviewed the code yet, this "overview" may take a different outlook after I review the code EDIT (after code review) |
cli/cli.go
Outdated
| client.registerHandlers() | ||
|
|
||
| return client | ||
| type response struct { |
There was a problem hiding this comment.
Permit me to saddle you with this requirement. This type should go. All commands should handle displaying appropriate responses after successful execution. Compare this comment. You may not want to stress too much about how to display the successful output for different commands, just print what is currently printed. Other PRs building off related issues will revise the successful message printing for each command. Examples are #49, #54 and #55
As provided for by the signature of Execute in goflags.Commander, commands should only return an error which is printed to the terminal.
There was a problem hiding this comment.
I think this should be done in a different PR. Take the balance handler: #49 will change the way the output is displayed. Same for the others. Since they're working on the output messages also, we can let them do it, or do it ourselves in a separate PR. Doing it now will require that I change the output myself (even if I do not handle formatting, I get to unnecessarily mess with what they're doing).
cli/commands.go
Outdated
| type SendCommand struct{} | ||
|
|
||
| func (s SendCommand) Execute(args []string) error { | ||
| res, err := normalSend(walletClient, args) |
There was a problem hiding this comment.
As mentioned in an earlier comment, the code in normalSend should come here. No need to call another handler function, this particular pattern is repeated across all Execute methods in this file, causing avoidable repetition.
Also, assuming SendCommand supports some command-specific options, we can have access to such option here seamlessly (s.option). Passing such options to the normalSend handler will be another stress that can be avoided.
There was a problem hiding this comment.
This comment applies to all Execute methods in this file. Also, it might make sense to put these in handlers.go, leaving commands.go as a file that describes the supported commands and handler.go as the file that actually perform the actions for each command.
main.go
Outdated
| _, err := parser.Parse() | ||
| if cli.IsFlagErrorType(err, flags.ErrCommandRequired) { | ||
| commands := supportedCommands(parser) | ||
| fmt.Fprintln(os.Stderr, "Available Commands: ", strings.Join(commands, ", ")) |
There was a problem hiding this comment.
Can we make supportedCommands return the joined string instead? The function isn't really used anywhere else so we can modify it to return what we actually need rather than it returning something and then we perform some other operation on the returned value.
Alternatively, we can just modify the supportedCommands function to print out the message and return nothing. The command can also be renamed accordingly.
There was a problem hiding this comment.
Supported commands are a list of commands, so a []string is appropriate.
There was a problem hiding this comment.
The motivation for this requirement is that supportedCommands isn't used or needed anywhere else. However, the function would likely find use soon (in #63) so let's leave it as is
|
@itswisdomagain The help message for command-specific help has been shortened. Here's what it looks like now. If that is acceptable, then command-specific help has been covered as required, leaving us with experimental commands to grok. |
To a very large extent, it does. The associated issue #51 recommended creating a new command Let's retain this, it's a welcome change. We might need to clean it up (I'm thinking, add a message below the command usage text that says how to see the options, something like |
Align the happy path to the left to improve readability.
main.go
Outdated
| fmt.Println(err) | ||
| return | ||
| } | ||
| if parser.Active == nil { |
There was a problem hiding this comment.
If you use else if here, the return in the preceding if block would be unnecessary
os.Exit(1) only if there command run returned an error.
os.Exit(1) only if the command run returned an error.
…to provide-command-types
Refactor commands to use the go-flags package properly.
Commands and subcommands can now be used in a simpler way.
EDIT: This also fixes #70