unjs / citty

🌆 Elegant CLI Builder
Other
775 stars 26 forks source link

Non positional arguments of main command usage does not allow to use any other commands #133

Open sausir1 opened 6 months ago

sausir1 commented 6 months ago

Describe the feature

Did not exactly knew is this a feature to implement, or a bug to fix, but marked as a feature.

Description

Seems like this part of the program could be improved - if main command have args and some sub-commands, citty fails to execute the underlying commands when any argument is passed. To better understand what I am trying to say is:

  1. I have a command that both have args that it accepts, and sub-commands that it can launch:
    
    const subCommand = defineCommand({
    meta: {
    name: "subcommand",
    version: "0.0.1",
    },
    args: {
    world: {
      type: "string",
      required: true,
    },
    },
    run(context) {
    console.log("sub: citizen of planet %s!", context.args.world);
    },
    });

const main = defineCommand({ meta: { name: "play", }, args: { name: { type: "string", required: false, }, }, setup(context) { console.log("main: hello %s", context.args?.name); }, subCommands: { subCommand, }, });

If I want to launch the subCommand I would do:
```sh
$ ./example.js subCommand --world "earth"

this works fine, but if i would like to launch this:

$ ./example.js --name "Citty" subCommand --world "earth"

This would fail with the error message, "Unknown command Citty".

I think this can be easily fixed by changing the logic where the next command's name is being parsed in command.ts file's runCommand method.

   ...
      const subCommandArgIndex = opts.rawArgs.findIndex(
        (arg) => !arg.startsWith("-"),
      );
  ....

I came up with this solution for getting the next command's index:

const subCommandArgIndex = opts.rawArgs.findIndex((arg, index) => {
        const currentIsNotArg = !arg.startsWith("-"); // checks that the current arg is not a argument
        if (index === 0) {
          return currentIsNotArg;
        }
        const prevArg = opts.rawArgs[index - 1];
        // if current `arg` is not an argument, then we need to check if this is a value of a previous `arg` argument or not.
        return (
          currentIsNotArg && !prevArg.startsWith("-") && arg in subCommands
        );
      });

P.S. There's still a caveat left here - if positional argument's value matches the command's name, command might get called with bad parameters.

It also seems that positional argument with default value followed by a sub-command is also not working. Is this the desired behavior? If so, maybe ArgDef type should be extended, to always require positional arguments? Or it this too much of an edge case?

Additional information

sausir1 commented 6 months ago

I've checked with all types of allowed argument types to date, seems like these changes addresses the issue, except that one with positional arguments