p-ranav / argparse

Argument Parser for Modern C++
MIT License
2.72k stars 250 forks source link

Report which option generates error #299

Closed jacg closed 1 year ago

jacg commented 1 year ago

For example, in an interface which admits usage such as

prog -a a-arg -b b-arg

if the user omits the -b, like this

prog -a a-arg b-arg

the program generates the somewhat cryptic (it's not too bad in this simplified context, but in more general real-world examples, it's not very helpful) message: "Maximum number of positional arguments exceeded".

Would it be possible to report which option is concerned?

In the toy example above, the message would be better if it were something like "Maximum number of positional arguments exceeded for option '-a/--number-of-apples'".

p-ranav commented 1 year ago

Hi,

Thanks for the feedback. I understand and agree that the generated output is somewhat cryptic.

Before I work on improving this, let me describe here why / what the argparse is trying to say here with its output.

DISCLAIMER: I am not trying to be defensive - I simply want to describe the WHY. I know it's not ideal in many cases and I'll work on improving it. No user should have to necessarily know the semantics of argparse's model.

Positional Argument

In argparse lingo, a positional argument is one that does not require any preceding option name. The value is defined purely by its position.

argparse::ArgumentParser program;
program.add_argument("a");
program.add_argument("b");
program.parse_args(argc, argv);

The above program accepts 2 positional arguments (mapping to the names "a" and "b")

The usage would be like so:

./prog 1 2

where a = 1 and b = 2

If one of these positional arguments is not provided, the output is like so:

./prog 1 
libc++abi: terminating due to uncaught exception of type std::runtime_error: b: 1 argument(s) expected. 0 provided.
zsh: abort      ./prog 1

Notice the exception b: 1 argument(s) expected. 0 provided. where the program tells you that a value was expected for the option b.

Optional Arguments

Your program constructs 2 optional arguments -a and -b. The usage for these arguments is typically -a <value> and/or -b <value>, i.e., the option name is to be provided.

It is not a positional argument for a number of reasons - one of which is the fact that its position is not important. Since the option name is provided, either of these usage patterns is OK: -a 1 -b 2 and -b 1 -a 2

In this case, when not providing -b and erroneously using the program like so:

./prog -a 1 2   
libc++abi: terminating due to uncaught exception of type std::runtime_error: Maximum number of positional arguments exceeded

argparse reports the error Maximum number of positional arguments exceeded since ZERO positional arguments were expected. The value 2 was interpreted as a value for a positional argument but no positional arguments were defined in the program.

p-ranav commented 1 year ago

All that said, can this be improved? I think so. I'll cook up a branch and see what I can do.

p-ranav commented 1 year ago

OK I've pushed some updates that should improve the error reporting a little.

Case 1

parser.add_argument("-a");
parser.add_argument("-b");

with usage

prog -a 1 2

will report Zero positional arguments expected, did you mean -b VAR

Case 2

parser.add_argument("-a", "--number-of-apples");
parser.add_argument("-b");

with usage

prog 1 2

will report Zero positional arguments expected, did you mean -a/--number-of-apples VAR

Case 3

parser.add_argument("-a");
parser.add_argument("-b");
parser.add_argument("c");

with usage

prog -a 1 2 3 4

will report Maximum number of positional arguments exceeded, failed to parse '3'.

jacg commented 1 year ago

Thanks for your explanation and implementation.

In my toolchain the previous commit (62052fefcb552e138a9d9e2807e883edcb09569a) introduces compilation errors related to mismatches between signed and unsigned integers, so I needed to apply this patch:

diff --git a/include/argparse/argparse.hpp b/include/argparse/argparse.hpp
index fa31ef0..164198b 100644
--- a/include/argparse/argparse.hpp
+++ b/include/argparse/argparse.hpp
@@ -457,12 +457,12 @@ int get_levenshtein_distance(const StringType &s1, const StringType &s2) {
   std::vector<std::vector<int>> dp(s1.size() + 1,
                                    std::vector<int>(s2.size() + 1, 0));

-  for (int i = 0; i <= s1.size(); ++i) {
-    for (int j = 0; j <= s2.size(); ++j) {
+  for (unsigned long i = 0; i <= s1.size(); ++i) {
+    for (unsigned long j = 0; j <= s2.size(); ++j) {
       if (i == 0) {
-        dp[i][j] = j;
+        dp[i][j] = static_cast<int>(j);
       } else if (j == 0) {
-        dp[i][j] = i;
+        dp[i][j] = static_cast<int>(i);
       } else if (s1[i - 1] == s2[j - 1]) {
         dp[i][j] = dp[i - 1][j - 1];
       } else {

In my original desrciption, for the sake of brevity, I didn't give the full details of our use case, which is more like this

args.add_argument("-a", "--aaa");
args.add_argument("-b", "--bbb").nargs(at_least_one).append();
args.add_argument("-c", "--ccc").nargs(at_least_one).append();
args.add_argument("-d", "--ddd").nargs(at_least_one).append();

After your fix, the erroneous CLI command

prog -a apple banana

gives the error

Zero positional arguments expected, did you mean -b/--bbb ITEMS...

which is already a significant improvement on what was emitted before. Thank you.

However,

In general, I appreciate that this that improving these messages any further is probably more trouble than it's worth.

Also, slight pedantry: "did you mean ..." is a question, so it should finish with a question mark.

Many thanks for the quick solution.

p-ranav commented 1 year ago

The mismatch between signed and unsigned should be fixed now (https://github.com/p-ranav/argparse/commit/ecccae530c85974d4686f4037181f574116de5ae and https://github.com/p-ranav/argparse/commit/b43c0a7e8344a08e8c805c43433599310fc9c740).

p-ranav commented 1 year ago

"did you mean '-b/--bbb banana'" is a good idea and worth attempting. I agree, mentioning all the options that accept values would've become a bit too verbose, especially in a longer-form CLI.

I also agree that did you mean is a question. I will add a question mark.