-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
cmd/tailscale/cli: fix typod subcommand error messages #11757
Conversation
efcc016
to
5b40424
Compare
cmd/tailscale/cli/cli.go
Outdated
@@ -216,11 +238,29 @@ var rootArgs struct { | |||
socket string | |||
} | |||
|
|||
func walkCommands(cmd *ffcli.Command, f func(*ffcli.Command)) { | |||
f(cmd) | |||
func walkCommands(cmd *ffcli.Command, parents []*ffcli.Command, f func(c *ffcli.Command, parents []*ffcli.Command)) { |
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.
the old signature was readable. this new signature probably warrants some docs on what's going on.
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 make the recursive part be a closure func inside this func and keep the outer func exposed to callers not taking the parents second argument that is always nil?
And maybe the func can take a new struct type containing the command (embedded?) and parents, rather than taking two things, especially because the second thing is often ignored by callers?
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've rewritten this function to use iteration because it was upsetting me that I couldn't reference the recursive closure from its own declaration: https://go.dev/play/p/JZcwMW7ttD0
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.
The way to write self-referential func literals is https://go.dev/play/p/18WqmO49FTF for better or worse
5b40424
to
3492096
Compare
3492096
to
d92efee
Compare
Updates #cleanup Signed-off-by: Paul Scott <paul@tailscale.com>
Updates #cleanup Signed-off-by: Paul Scott <paul@tailscale.com>
Updates #cleanup Signed-off-by: Paul Scott <paul@tailscale.com>
d92efee
to
8d0aa1d
Compare
2ac01d3
to
f521ffe
Compare
Fixes #11672
Updates #11626
This PR changes the error when a user typos a subcommand or fails to provide one for the following commands:
The lesson about ffcli to take away from this is that the closest valid parent's Exec function gets run if the subcommand is not recognised. So those
Exec: func(...) { if len(args) > 1 { return errors.New("unexpected arg") } }
andExec: func(...) { return errors.New("subcmd required") }
were catching all of the typos.❓ Do we want to default to printing
--help
when a subcommand is required buy omitted? Sometimes that help text can be quite long. I've opted for writing a short "tailscale file: subcommand required: cp, get"-style error message at the moment, but it's easy for us to change this.There's a few other tidy-ups in here because I wanted the
walkCommands
change from #11336 so that we could generate better errors (which itself came with some unit test improvements and fixes). The commits are granular, but I can split up the PR further if needed!Examples:
Changes to
--help
: