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.

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

Incorrect handling of nested subcommands and command name removal #41

Open
JosephGalante opened this issue Sep 6, 2023 · 0 comments
Open

Comments

@JosephGalante
Copy link

JosephGalante commented Sep 6, 2023

Issue Description:

In cli/args/args.go, The current implementation of the code does not correctly handle nested subcommands and fails to remove the command name from the arguments, leading to potential issues in command parsing. This issue aims to address these problems and improve the overall code quality.

Steps to Reproduce:

  1. Create a scenario with nested subcommands, where you have multiple levels of subcommands.
  2. Execute commands that involve nested subcommands and observe the behavior.

Expected Behavior:

  1. Nested subcommands should be handled correctly, with the command name removed from the arguments.
  2. The sequence of parsing subcommands and processing arguments should be correct.

Actual Behavior:

  1. Nested subcommands are not handled correctly.
  2. The command name is not removed from the arguments, leading to incorrect parsing.

Proposed Changes:

  • Update the parseCommandArguments function to use the new helper function for finding the index of a command or its alias in the arguments.
  • Revise the MovePostFixCommands function to correctly handle nested subcommands and ensure the command name is removed from the arguments.

In a very rough draft, not intended to be perfect, here is how I believe parseCommandArguments() could be improved:

func parseCommandArguments(commands []*cli.Command, args ...string) []string {
    if len(args) <= 1 || len(commands) == 0 {
        return args
    }

    // Helper function to find the index of a command or its alias in args
    findCommandIndex := func(cmd *cli.Command) int {
        for _, name := range append([]string{cmd.Name}, cmd.Aliases...) {
            idx := IndexOf(args, name)
            if idx != -1 {
                return idx
            }
        }
        return -1
    }

    // Iterate through commands
    for _, cmd := range commands {
        idx := findCommandIndex(cmd)

        if idx != -1 {
            // Parse command flags and arguments
            cmdArgs := append([]string{cmd.Name}, args[idx+1:]...)
            cmdArgs = append(cmdArgs, MovePostFixCommands(args[:idx], cmd.Subcommands)...)
            args = cmdArgs

            return args
        }
    }

    return args
}

and below is how I believe, on a very rough level, again, not intended to be perfect, how we would potentially improve MovePostFixCommands():

// MovePostFixCommands places the command being called at the beginning of the args slice for parsing
func MovePostFixCommands(args []string, commands []*cli.Command) []string {
    cmdArgs := []string{}
    cmdName := ""

    // Find which command is being called
    for _, cmd := range commands {
        argName := cmd.Name
        idx := IndexOf(args, argName)
        if idx == -1 {
            for _, alias := range cmd.Aliases {
                idx = IndexOf(args, alias)
                if idx != -1 {
                    argName = alias
                    break
                }
            }
        }
        if idx != -1 {
            // Set the command name
            cmdName = argName

            // Remove argName from args
            args = append(args[:idx], args[idx+1:]...)

            // Recursively handle subcommands
            args = MovePostFixCommands(args, cmd.Subcommands)
            break
        }
    }

    if cmdName != "" {
        cmdArgs = append(cmdArgs, cmdName)
    }

    return append(cmdArgs, args...)
}

Obviously, this is just a rough draft, and I'm sure there are better ways to implement this. I'm also not sure if this is the best way to handle nested subcommands, but I believe it is a step in the right direction.

Additionally, the ParseArguments() function would remain unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant