andrey-zherikov / argparse

Parser for command-line arguments
https://andrey-zherikov.github.io/argparse/
Boost Software License 1.0
30 stars 6 forks source link

Subcommands do not mix well with NamedArguments #78

Closed gizmomogwai closed 9 months ago

gizmomogwai commented 1 year ago
import std;
import argparse;

@Command("c1")
struct command1
{
    bool c1data;
}
@Command("c2")
struct command2
{
    @NamedArgument
    bool c2data;
}
struct Program {
    @NamedArgument("numbers")
    bool numbers;
    SumType!(command1, command2) cmd;
}

void _main(Program args)
{
    writeln(args);
}
mixin CLI!(Program).main!((arguments) {
    _main(arguments);
});

This program does not show the subcommand when called with --help:

Usage: argparse-test [--numbers] [-h]

Optional arguments:
  --numbers
  -h, --help    Show this help message and exit

If I remove the @NamedArgument annotation, then its printing the expected output:

Usage: argparse-test [--numbers] [-h] <command> [<args>]

Available commands:
  c1
  c2

Optional arguments:
  --numbers
  -h, --help    Show this help message and exit
gizmomogwai commented 1 year ago

p.s. version used: 1.2.0

andrey-zherikov commented 1 year ago

Please try adding @SubCommands:

struct Program {
    @NamedArgument("numbers")
    bool numbers;
    @SubCommands
    SumType!(command1, command2) cmd;
}
gizmomogwai commented 1 year ago

I experimented a little more on my problem and could boil it down to actually another "bad" feature interaction. A small example showing my problem is this:

#!/usr/bin/env dub
/+ dub.sdl:
   name "mindfile"
   dependency "argparse" version="1.2.0"
 +/
module argparse_test;
import std;
import argparse;

@Command("c1")
struct command1
{
    @PositionalArgument(0)
    string numbers;
    @NamedArgument
    bool c1data;
}
@Command("c2")
struct command2
{
    @PositionalArgument(0)
    string numbers;
    @NamedArgument
    bool c2data;
}
@Command
struct Program {
    @SubCommands SumType!(command1, command2) cmd;
}

void _main(Program args)
{
    writeln(args);
}

mixin CLI!(Program).main!((arguments) {
    _main(arguments);
});

both helptexts (for the program itself and for the subcommands work beautifully:

(ldc-1.30.0)christian.koestlin@AMAFVFF202SQ05P ~/S/p/_/a/argparse-test (master)> ./argparse-test.d --help
Usage: mindfile [-h] <command> [<args>]

Available commands:
  c1
  c2

Optional arguments:
  -h, --help    Show this help message and exit

(ldc-1.30.0)christian.koestlin@AMAFVFF202SQ05P ~/S/p/_/a/argparse-test (master)> ./argparse-test.d c1 --help
Usage: mindfile c1 [--c1data] [-h] numbers

Required arguments:
  numbers

Optional arguments:
  --c1data
  -h, --help    Show this help message and exit

As soon as I add Default! to one of the subcommands the helptexts break down and also the commandline processing:

#!/usr/bin/env dub
/+ dub.sdl:
   name "mindfile"
   dependency "argparse" version="1.2.0"
 +/
module argparse_test;
import std;
import argparse;

@Command("c1")
struct command1
{
    @PositionalArgument(0)
    string numbers;
    @NamedArgument
    bool c1data;
}
@Command("c2")
struct command2
{
    @PositionalArgument(0)
    string numbers;
    @NamedArgument
    bool c2data;
}
@Command
struct Program {
    @SubCommands SumType!(Default!command1, command2) cmd;
}

void _main(Program args)
{
    writeln(args);
}

mixin CLI!(Program).main!((arguments) {
    _main(arguments);
});
(ldc-1.30.0)christian.koestlin@AMAFVFF202SQ05P ~/S/p/_/a/argparse-test (master)> ./argparse-test.d --help
Usage: mindfile [-h] <command> [<args>]

Available commands:
  c1
  c2

Optional arguments:
  -h, --help    Show this help message and exit

(ldc-1.30.0)christian.koestlin@AMAFVFF202SQ05P ~/S/p/_/a/argparse-test (master)> ./argparse-test.d c1 --help
Usage: mindfile [-h] <command> [<args>]

Available commands:
  c1
  c2

Optional arguments:
  -h, --help    Show this help message and exit

(ldc-1.30.0)christian.koestlin@AMAFVFF202SQ05P ~/S/p/_/a/argparse-test (master)> ./argparse-test.d c1 number
Error: Unrecognized arguments: ["number"]
Error Program exited with code 1
andrey-zherikov commented 1 year ago

As soon as I add Default! to one of the subcommands the helptexts break down and also the commandline processing:

Finally got some time to look at this. This case is ambiguous a bit: should c1 be treated as a subcommand or a value for command1.numbers? argparse does the latter right now.

There is a bit more complex case - when command1 has multiple positional arguments:

struct command1
{
    @PositionalArgument(0)
    string numbers;
    @PositionalArgument(1)
    string numbers1;
    @NamedArgument
    bool c1data;
}

How should parser behave for program some_value c1 command line? Should c1 be a value for command1.numbers1 or be a subcommand?

My thoughts are:

This is applicable to the case when a program allows only one subcommand in command line. Unimplemented feature (it's in my todo list) - where a program allows multiple subcommands in a command line - will treat unnamed argument as a new subcommand if it matches subcommand name regardless to arguments' position in command line.

gizmomogwai commented 1 year ago

Hi @andrey-zherikov, thanks for looking into this.

My personal opinion is, that the parser should be more strict in when arguments of commands or subcommands are accepted. Even named parameters of a "parent" command should not be valid, after the subcommand was started.

For Positional arguments kind of the same, but I would even go further in that I would only accept positional arguments for the last subcommand in a chain, if you know what I mean ... or if positional arguments are allowed everywhere I would associate them with the currently active command.

But for sure this will change behavior + it might be possible, that with that something like a find would not be possible to implement (although I have to admit, that I can only use the most basic invocations of find (I think its a counter example of a good commandline tool)).

On a sidenote, normally I do not like positional arguments that much (they are shorter to type (without a --name in front), but harder to understand.

One thing about the Default! ... perhaps it's only possible to work with default, when all values are kind on default ... if thats not the case, one would need to put all parameters and commands into the comandline ...

Otherwise your library is just the best!

andrey-zherikov commented 10 months ago

I've been working on rewriting the parser lately and was thinking about how subcommands (default and not) and argument parsing should work together. Finally i've reached out the conclusion below which (I hope) makes sense.

Given that argparse has a feature of marking positional arguments optional (vs. required), there will be the following restrictions added:

These restrictions allow having the following parsing rules for positional arguments:

I have code with unit tests almost ready, just need to clean it up. I'll link it here as soon as I have PR ready.

andrey-zherikov commented 10 months ago

Applying this approach to the use case above (with default subcommand):

>tool --help
Usage: tool [-h] <command> [<args>]

Available commands:
  c1
  c2

Optional arguments:
  -h, --help    Show this help message and exit
>tool c1 --help
Usage: tool c1 [--c1data] [-h] numbers

Required arguments:
  numbers

Optional arguments:
  --c1data
  -h, --help    Show this help message and exit
>tool c2 --help
Usage: tool c2 [--c2data] [-h] numbers

Required arguments:
  numbers

Optional arguments:
  --c2data
  -h, --help    Show this help message and exit
>tool c1 number
Program(Default!(command1)(command1("number", false)))
gizmomogwai commented 10 months ago

Good to hear that you are improving your lib!!! Please tell me if you have something to try out, I might upgrade early!

andrey-zherikov commented 10 months ago

I'm sorry that it takes so long. Here is working branch: https://github.com/andrey-zherikov/argparse/tree/parser-rework - I might do some changes before creating PR but you can try it out. Note that ldc 1.35 fails to compile code on Windows

gizmomogwai commented 10 months ago

I just did a quick tests, and my program for which I updated argparse to your branch still works good enough, so its not much of api breakage. I only had problems with the comparing the color argument against Config.StylingMode.on.

Another thing I saw now, is that I do use one positional argument with a name. The helptext shows this as required (in my case its not really required as it defaults to ab empty string which is also ok).

Otherwise really nice .. especially the new readme looks very clean!

andrey-zherikov commented 10 months ago

I'm working on new major release so breaking changes are expected (they all will be listed in changelog). As of now, I have couple of pending changes in API before publishing new version. I would also like to migrate readme to wiki as it becomes huge but I'd rather not depend new release on it.

gizmomogwai commented 10 months ago

Sure .. no problem to break a little API ... Thats expected from a new major version.

andrey-zherikov commented 9 months ago

PR #128 is merged so this is fixed and will go live in 2.0 release