mkideal / cli

CLI - A package for building command line app with go
MIT License
732 stars 43 forks source link

Handle positional arguments when a command doesn't have subcommands, and the user doesn't provide flags #61

Closed mailund closed 3 years ago

mailund commented 3 years ago

I am not sure, but is it possible to give a command positional arguments without adding flags? I have a couple of commands in an application I was trying to move to this framework, and I ran into some problems.

In this simple example, I just try to print positional arguments:

package main

import (
    "fmt"
    "os"

    "github.com/mkideal/cli"
)

type argMain struct {
    cli.Helper
    Why bool `cli:"why"`
}

func main() {
    var main = &cli.Command{
        Name: "test",
        Argv: func() interface{} { return new(argMain) },
        Fn: func(ctx *cli.Context) error {
            ctx.String("args: '%+v'\n", ctx.Args())
            return nil
        },
    }

    if err := main.Run(os.Args[1:]); err != nil {
        fmt.Fprintln(os.Stderr, err)
        os.Exit(1)
    }
}

If I compile and run the tool I get an error, indicating that the framework thinks that positional arguments are subcommands

⟩ go build -o test
⟩ ./test xxx yyy zzz
ERR! command xxx yyy zzz not found

When I look at Command.prepare(), I can see why:

func (cmd *Command) prepare(clr color.Color, args []string, writer io.Writer, resp http.ResponseWriter, httpMethods ...string) (ctx *Context, suggestion string, err error) {
    // split args
    router := []string{}
    for _, arg := range args {
        if strings.HasPrefix(arg, dashOne) {
            break
        }
        router = append(router, arg)
    }
    path := strings.Join(router, " ")
    child, end := cmd.SubRoute(router)

       ...
}

The first part of parsing tries to build a path to a command, and it doesn't stop until it sees a dash. And here, there isn't any dashes. So, prepare() concludes that I am trying to invoke a command that isn't registered. It's right on the second part, not so much on the first.

I can get positional arguments, though. I just need a flag first. If you were wondering what the Why argument did in the code above, this should demonstrate it:

⟩ ./test --why xxx yyy zzz
args: '[xxx yyy zzz]'

The parsing stops when it sees --why, so the arguments after the flag are parsed to the command just fine. The test in context_test.go also has a flag in front of free arguments, which is why it passes. It would fail without it, as far as I can see. But it seems a bit silly to have a flag just for that...

 I tried modifying the prepare() function so it checks if the child it finds while routing actually does have subcommands, and if not, it doesn't report an error. The tests in the package runs, plus a new that checks for positional arguments, but of course I don't know if there are other cases where the change might break something.

suntong commented 3 years ago

@mkideal, thoughts?

@mailund, have you tried to make use of the solution in #10 yet?

suntong commented 3 years ago

Also, @mailund, there is a NumArg setting, just like NumOption: in #10, have you tried that yet?

mailund commented 3 years ago

I haven't tried it, but yes I believe that specifying the command such that it can subroute might fix the issue. Then it might not see positional arguments as subcommands that aren't there. A bit weird to make commands into subcommands that way, to give them positions arguments, though.

The issue is that positional arguments work as expected if you provide at least one flag first, but if you don't, then by default the positional arguments are interpreted as commands. In my example, if you provide the --why flag, the following arguments go into args, but if you don't, then they are considered a path to a subcommand. It's an unexpected, to me at least, side effect of an optional flag.

That being said, it is not simple to combine subcommands and variadic positional arguments. A fixed number of required positional arguments are always easy to parse, because you know how many there are and any subcommand that follows have to come after. A variadic number of arguments have to come as a leaf-command in the subcommand tree to avoid confusion. That is why my patch simply checked if a command has children. If it doesn't, then arguments can safely be interpreted as positional args for the command.

suntong commented 3 years ago

Have you looked at the NumArg setting @mailund?

mailund commented 3 years ago

Yes, that doesn't change anything. With

package main

import (
    "fmt"
    "os"

    "github.com/mkideal/cli"
)

type argMain struct {
    cli.Helper
    Why bool `cli:"why"`
}

func main() {
    var main = &cli.Command{
        Name:   "test",
        NumArg: cli.AtLeast(0),
        Argv:   func() interface{} { return new(argMain) },
        Fn: func(ctx *cli.Context) error {
            ctx.String("args: '%+v'\n", ctx.Args())
            return nil
        },
    }

    if err := main.Run(os.Args[1:]); err != nil {
        fmt.Fprintln(os.Stderr, err)
        os.Exit(1)
    }
}

you still get:

⟩ ./prog foo bar
ERR! command foo bar not found

when you don't provide the flag, but expected behaviour when you do:

⟩ ./prog --why foo bar
args: '[foo bar]'

The run function doesn't look at the command before it fails. It concludes that you have a path to a command that doesn't exist (because it includes the positional arguments to the path, although it shouldn't), so it fails before it gets hold of the command where you specify the number of arguments. At least from what I can quickly glance from code, but I might be wrong. It definitely fails.

Setting CanSubRoute: true fixes this (I just checked), but I think it is a slightly strange design to have to say that a command is a subcommand that can sub route to get it to behave as a command without subcommands...

suntong commented 3 years ago

Ok, let's wait for @mkideal's response then. At least you have something that works in the meantime.

mailund commented 3 years ago

Well, I went with implementing my own package for it, that treats flags and positional arguments the same way, so I am not waiting for a fix. And yes, CanSubRoute: true would work as a fix if I were.

Cheers

mkideal commented 3 years ago

It's a good idea to solve it! thx @mailund