Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(args): Correct legacyArgs logical errors #1157

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(args): Correct legacyArgs logical errors #1157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01ce53c
fix(args): Correct `legacyArgs` logical errors
Jul 11, 2020
f5ce802
Modify the logic of Command execute function
Jul 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions args.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,15 @@ type PositionalArgs func(cmd *Command, args []string) error

// Legacy arg validation has the following behaviour:
// - root commands with no subcommands can take arbitrary arguments
// - root commands with subcommands will do subcommand validity checking
// - subcommands will always accept arbitrary arguments
// - commands with subcommands cannot take any arguments
func legacyArgs(cmd *Command, args []string) error {
// no subcommand, always take args
if !cmd.HasSubCommands() {
return nil
}

// root command with subcommands, do subcommand checking.
if !cmd.HasParent() && len(args) > 0 {
// root command with subcommands, the args should be empty.
if cmd.HasSubCommands() && len(args) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot make this change as it will break backwards-compatibility.
Cobra does support commands that take args and sub-commands.
For example helm v2 has the helm get command:

Usage:
  helm get [flags] RELEASE_NAME
  helm get [command]

As you can see you can either pass it a sub-command, or you can pass it an argument.
As you mention, this can cause confusion so helm v3 no longer does that.
However it shows that some programs do use this feature and therefore we cannot change it.

return fmt.Errorf("unknown command %q for %q%s", args[0], cmd.CommandPath(), cmd.findSuggestions(args[0]))
}
return nil
Expand Down
12 changes: 6 additions & 6 deletions command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,12 +801,6 @@ func (c *Command) execute(a []string) (err error) {
}
}

if !c.Runnable() {
return flag.ErrHelp
}

c.preRun()

argWoFlags := c.Flags().Args()
if c.DisableFlagParsing {
argWoFlags = a
Expand All @@ -816,6 +810,12 @@ func (c *Command) execute(a []string) (err error) {
return err
}

if !c.Runnable() {
return flag.ErrHelp
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this after the parsing of the arguments seems right to me. The change will only impact commands that are not runnable which have the Args field defined. The change means that the Args field will be respected when it used to not be. I believe this can be justified as a change.


c.preRun()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more worrisome. I don't understand enough about preRun() and initializers to know if a program would expect the preRun() to be executed even if the call to Args fails with an error, which used to happen before but will not with this change.

This could be a backwards-compatibility issue.
@jharshman do you think this could break backwards-compatibility?


for p := c; p != nil; p = p.Parent() {
if p.PersistentPreRunE != nil {
if err := p.PersistentPreRunE(c, argWoFlags); err != nil {
Expand Down