fix for parsing error handling of an empty command argument #48

jolly-fellow · 2023-04-25T05:32:34Z

Example: antler-proj build ~/path/proj ""
corresponding to issue #19

mikelik

Well it's a hack, but it works. I would just simplify catching the exception

mikelik · 2023-04-25T13:08:22Z

tools/main.cpp

+   catch (CLI::ExtrasError &e) {
+      if (std::string_view{"The following argument was not expected: "} == e.what()) {
+         return app.exit(CLI::ExtrasError("Empty argument is encountered in the command line", e.get_exit_code()));
+      }


According to https://github.com/AntelopeIO/antler-proj/blob/main/tools/CLI11.hpp#L902 ExtrasError always contain either The following arguments were or The following arguments was

Suggested change

catch (CLI::ExtrasError &e) {

if (std::string_view{"The following argument was not expected: "} == e.what()) {

return app.exit(CLI::ExtrasError("Empty argument is encountered in the command line", e.get_exit_code()));

}

catch (const CLI::ExtrasError& e) {

return app.exit(CLI::ExtrasError("Empty argument is encountered in the command line", e.get_exit_code()));

}

According to https://github.com/AntelopeIO/antler-proj/blob/main/tools/CLI11.hpp#L902 ExtrasError always contain either The following arguments were or The following arguments was

But we should not change the behavior in case if this exception thrown for non empty arguments. In this case all these arguments should be printed as "The following arguments were not expected: arg1 arg2 etc." or "The following argument was not expected: arg"
We should change the message only in case if it prints "The following argument was not expected: " which happens only if we have one "" (empty argument) in the command line.

Ah, I see. This is bigger hack than I thought.
Isn't there any other way to fix it?
I.e. checking arguments before parsing if any of them is ""

These are "extra" arguments - so they aren't caught by the standard validators.

I've suggested a change (below) that could address this.

Ah, I see. This is bigger hack than I thought. Isn't there any other way to fix it? I.e. checking arguments before parsing if any of them is ""

This is the best solution which I can invent without changing of the CLI11.hpp code.
The problem is not with empty arguments of options, the problem with empty options.
The parser recognize the subcommand, then find all its options and their arguments, and all other text which it can find the parser consider as "extra" arguments, i.e. trash which it shows in the text of the exception when prints the error. All it works if these extra arguments have symbols. But it seems authors of the parser don't know that it possible to declare an empty argument in the command line, so the parser just don't handle this case at all and print nothing instead of name of an argument. In other words everything which is not looks like a known subcommand or option of a subcommand or a parameter of an option considers as an "extra" argument.

mikelik · 2023-04-25T14:03:40Z

tools/main.cpp

+         return app.exit(CLI::ExtrasError("Empty argument is encountered in the command line", e.get_exit_code()));
+      }
+   }
+   catch (const CLI::ParseError &e) {


other references in code are similar to this:

Suggested change

catch (const CLI::ParseError &e) {

catch (const CLI::ParseError& e) {

I agree - @mikelik is suggesting the "correct" style.

ScottBailey · 2023-04-25T17:44:58Z

tools/main.cpp

+   // This hack fix parsing error handling of an empty command argument. Example: antler-proj build ~/path/proj ""
+   catch (CLI::ExtrasError &e) {
+      if (std::string_view{"The following argument was not expected: "} == e.what()) {
+         return app.exit(CLI::ExtrasError("Empty argument is encountered in the command line", e.get_exit_code()));
+      }
+   }


Missing an app.exit(e).

Also, I agree with @mikelik - we should handle both "was" and "were". Here's a possibility:

Suggested change

// This hack fix parsing error handling of an empty command argument. Example: antler-proj build ~/path/proj ""

catch (CLI::ExtrasError &e) {

if (std::string_view{"The following argument was not expected: "} == e.what()) {

return app.exit(CLI::ExtrasError("Empty argument is encountered in the command line", e.get_exit_code()));

}

}

// Handle empty command argument. Example: `antler-proj build ~/path/proj ""`

catch (CLI::ExtrasError& e) {

std::string what = e.what();

// trim space

while(!what.empty() && what.back() == ' ')

what.pop_back();

if( what == "The following argument was not expected:" || what == "The following arguments were not expected:") {

return app.exit(CLI::ExtrasError("Empty argument(s) encountered in the command line.", e.get_exit_code()));

}

app.exit(e);

}

Scott is right, if one would pass 2+ empty arguments (which is awkward but possible due to errors in scripts) - it will fail.
But his solution seems to me too verbose and hard to understand from glance. here is mine:

Suggested change

// This hack fix parsing error handling of an empty command argument. Example: antler-proj build ~/path/proj ""

catch (CLI::ExtrasError &e) {

if (std::string_view{"The following argument was not expected: "} == e.what()) {

return app.exit(CLI::ExtrasError("Empty argument is encountered in the command line", e.get_exit_code()));

}

}

// This hack fix parsing error handling of an empty command argument. Example: antler-proj build ~/path/proj ""

catch (CLI::ExtrasError &e) {

const std::regex rex("The following argument(s)? (was|were)? not expected:\\s+$", std::regex_constants::ECMAScript);

if (std::regex_search(e.what(), rex)) {

return app.exit(CLI::ExtrasError("Unexpected empty argument is encountered in the command line", e.get_exit_code()));

}

}

Scott is right, if one would pass 2+ empty arguments (which is awkward but possible due to errors in scripts) - it will fail. But his solution seems to me too verbose and hard to understand from glance. here is mine:

I'm not against of such comprehensive check but if you try to test the cases with more than one empty argument you will see that they processed by the parser earlier and as I understand such exception will be never thrown for antler-proj command syntax.
And I prefer @mikelik and @ScottBailey solutions. Don't want to include a complex library for such simple cases.

ScottBailey

Must add app.exit(e) to correctly report other errors. Should behave the same when 1 empty argument or 2+ are provided.

Where's the test? A simple one could be added in tools/tests.

dimas1185 · 2023-04-26T17:59:55Z

tools/main.cpp

@@ -57,7 +57,7 @@ int main(int argc, char** argv) {
   app.add_flag_callback("-V,--version",
         [&app]() {
            std::cout << app.get_name() << " v" << antler::system::version::full() << std::endl;
-            std::exit(0);       // Succesfull exit MUST happen here.


now I feel relief that native English speakers also mess double letters in that word lol

jolly-fellow · 2023-04-27T05:03:43Z

I have added @dimas1185 update from main to avoid a merge collision.

tools/main.cpp

Co-authored-by: mikelik <mikelik@users.noreply.github.com>

ScottBailey

lgtm

fix for parsing error handling of an empty command argument

ad76552

jolly-fellow requested review from ScottBailey, mikelik and larryk85 April 25, 2023 05:32

jolly-fellow self-assigned this Apr 25, 2023

mikelik suggested changes Apr 25, 2023

View reviewed changes

jolly-fellow marked this pull request as ready for review April 25, 2023 14:29

ScottBailey reviewed Apr 25, 2023

View reviewed changes

ScottBailey suggested changes Apr 25, 2023

View reviewed changes

BenjaminGormanPMP linked an issue Apr 26, 2023 that may be closed by this pull request

Incorrect processing of an empty parameter. #19

Closed

dimas1185 reviewed Apr 26, 2023

View reviewed changes

Updated after review

be891e6

jolly-fellow requested review from dimas1185, mikelik and ScottBailey April 27, 2023 05:04

Updated after review

d909956

mikelik approved these changes Apr 27, 2023

View reviewed changes

tools/main.cpp Outdated Show resolved Hide resolved

Update tools/main.cpp

ca504b2

Co-authored-by: mikelik <mikelik@users.noreply.github.com>

ScottBailey approved these changes Apr 27, 2023

View reviewed changes

ScottBailey merged commit 83b0f1b into main Apr 27, 2023

ScottBailey deleted the empty_param_error_fix branch April 27, 2023 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for parsing error handling of an empty command argument #48

fix for parsing error handling of an empty command argument #48

jolly-fellow commented Apr 25, 2023

mikelik left a comment

mikelik Apr 25, 2023 •

edited

Loading

jolly-fellow Apr 25, 2023

mikelik Apr 25, 2023

ScottBailey Apr 25, 2023 •

edited

Loading

jolly-fellow Apr 27, 2023 •

edited

Loading

mikelik Apr 25, 2023

ScottBailey Apr 25, 2023

ScottBailey Apr 25, 2023 •

edited

Loading

dimas1185 Apr 26, 2023 •

edited

Loading

jolly-fellow Apr 27, 2023 •

edited

Loading

ScottBailey left a comment

dimas1185 Apr 26, 2023

jolly-fellow commented Apr 27, 2023

ScottBailey left a comment

	catch (const CLI::ParseError &e) {
	catch (const CLI::ParseError& e) {

-   // This hack fix parsing error handling of an empty command argument. Example: antler-proj build ~/path/proj ""
-   catch (CLI::ExtrasError &e) {
-      if (std::string_view{"The following argument was not expected: "} == e.what()) {
-         return app.exit(CLI::ExtrasError("Empty argument is encountered in the command line", e.get_exit_code()));
-      }
-   }
+   // Handle empty command argument. Example: `antler-proj build ~/path/proj ""`
+   catch (CLI::ExtrasError& e) {
+      std::string what = e.what();
+      // trim space
+      while(!what.empty() && what.back() == ' ')
+         what.pop_back();
+      if( what == "The following argument was not expected:" || what == "The following arguments were not expected:") {
+         return app.exit(CLI::ExtrasError("Empty argument(s) encountered in the command line.", e.get_exit_code()));
+      }
+      app.exit(e);
+   }

fix for parsing error handling of an empty command argument #48

fix for parsing error handling of an empty command argument #48

Conversation

jolly-fellow commented Apr 25, 2023

mikelik left a comment

Choose a reason for hiding this comment

mikelik Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

jolly-fellow Apr 25, 2023

Choose a reason for hiding this comment

mikelik Apr 25, 2023

Choose a reason for hiding this comment

ScottBailey Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

jolly-fellow Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

mikelik Apr 25, 2023

Choose a reason for hiding this comment

ScottBailey Apr 25, 2023

Choose a reason for hiding this comment

ScottBailey Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

dimas1185 Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

jolly-fellow Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

ScottBailey left a comment

Choose a reason for hiding this comment

dimas1185 Apr 26, 2023

Choose a reason for hiding this comment

jolly-fellow commented Apr 27, 2023

ScottBailey left a comment

Choose a reason for hiding this comment

mikelik Apr 25, 2023 •

edited

Loading

ScottBailey Apr 25, 2023 •

edited

Loading

jolly-fellow Apr 27, 2023 •

edited

Loading

ScottBailey Apr 25, 2023 •

edited

Loading

dimas1185 Apr 26, 2023 •

edited

Loading

jolly-fellow Apr 27, 2023 •

edited

Loading