Merge pull request #1178 from smehringer/argument_parser_fix

5 small argument parser fixes

include/seqan3/argument_parser/argument_parser.hpp

@@ -287,6 +287,13 @@ class argument_parser
                                std::string const & desc,
                                validator_type validator = validator_type{}) // copy to bind rvalues
     {
+        if (has_positional_list_option)
+            throw parser_design_error{"You added a positional option with a list value before so you cannot add "
+                                      "any other positional options."};
+
+        if constexpr (SequenceContainer<option_type> && !std::Same<option_type, std::string>)
+            has_positional_list_option = true; // keep track of a list option because there must be only one!
+
         // copy variables into the lambda because the calls are pushed to a stack
         // and the references would go out of scope.
         std::visit([=, &value] (auto & f) { f.add_positional_option(value, desc, validator); }, format);
@@ -492,6 +499,9 @@ class argument_parser
     //!\brief Keeps track of whether the parse function has been called already.
     bool parse_was_called{false};
 
+    //!\brief Keeps track of whether the user has added a positional list option to check if this was the very last.
+    bool has_positional_list_option{false};
+
     //!\brief Set on construction and indicates whether the developer deactivates the version check calls completely.
     bool version_check_dev_decision{};
 

include/seqan3/argument_parser/detail/format_base.hpp

@@ -247,7 +247,10 @@ class format_help_base : public format_base
             if (!(spec & option_spec::HIDDEN) && (!(spec & option_spec::ADVANCED) || show_advanced_options))
                   derived_t().print_list_item(prep_id_for_help(short_id, long_id) +
                                               " " + option_type_and_list_info(value),
-                                              desc + detail::to_string(" Default: ", value, ". ") +
+                                              desc +
+                                              ((spec & option_spec::REQUIRED)
+                                                  ? std::string{" "}
+                                                  : detail::to_string(" Default: ", value, ". ")) +
                                               validator.get_help_page_message());
         });
     }
@@ -291,7 +294,11 @@ class format_help_base : public format_base
             ++positional_option_count;
             derived_t().print_list_item(detail::to_string("\\fBARGUMENT-", positional_option_count, "\\fP ",
                                                           option_type_and_list_info(value)),
-                                        desc + detail::to_string(" Default: ", value, ". ") +
+                                        desc +
+                                        // a list at the end may be empty and thus have a default value
+                                        ((SequenceContainer<option_type> && !std::Same<option_type, std::string>)
+                                            ? detail::to_string(" Default: ", value, ". ")
+                                            : std::string{" "}) +
                                         validator.get_help_page_message());
         });
     }

include/seqan3/argument_parser/detail/format_parse.hpp

@@ -683,8 +683,7 @@ class format_parse : public format_base
 
         if (SequenceContainer<option_type> && !std::is_same_v<option_type, std::string>) // vector/list will be filled with all remaining arguments
         {
-            if (positional_option_count != (positional_option_calls.size()))
-                throw parser_design_error("Lists are only allowed as the last positional option!");
+            assert(positional_option_count == positional_option_calls.size()); // checked on set up.
 
             while (it != argv.end())
             {

include/seqan3/argument_parser/detail/version_check.hpp

@@ -215,7 +215,7 @@ class version_checker
                               "_" +
                               version +              // !user input! escaped on construction of the version_checker
 #if defined(_WIN32)
-                              "; exit  [int] -not $?}\" > nul 2>&1"
+                              "; exit  [int] -not $?}\" > nul 2>&1";
 #else
                               " > /dev/null 2>&1";
 #endif
@@ -341,7 +341,7 @@ class version_checker
         // nor did the the cookie tell us what to do. We will now ask the user if possible or do the check by default.
         write_cookie("ASK"); // Ask again next time when we read the cookie, if this is not overwritten.
 
-        if (detail::is_terminal())
+        if (detail::is_terminal()) // LCOV_EXCL_START
         {
             std::cerr << R"(
 #######################################################################
@@ -403,7 +403,7 @@ class version_checker
 )";
             return true; // default: check version if you cannot ask the user
         }
-    }
+    } // LCOV_EXCL_STOP
 
     //!\brief The identification string that may appear in the version file if an app is unregistered.
     static constexpr std::string_view unregistered_app = "UNREGISTERED_APP";

test/unit/argument_parser/argument_parser_design_error_test.cpp

@@ -70,8 +70,7 @@ TEST(parse_test, parser_design_error)
     std::vector<int> vec;
     argument_parser parser7{"test_parser", 4, argv2};
     parser7.add_positional_option(vec, "oh oh list not at the end.");
-    parser7.add_positional_option(option_value, "desc.");
-    EXPECT_THROW(parser7.parse(), parser_design_error);
+    EXPECT_THROW(parser7.add_positional_option(option_value, "desc."), parser_design_error);
 
     // using h, help, advanced-help, and export-help
     argument_parser parser8{"test_parser", 1, argv};
@@ -105,8 +104,8 @@ TEST(parse_test, parse_called_twice)
 {
     std::string option_value;
 
-    const char * argv[] = {"./argument_parser_test", "-s", "option_string"};
-    argument_parser parser{"test_parser", 3, argv};
+    const char * argv[] = {"./argument_parser_test", "--version-check", "0", "-s", "option_string"};
+    argument_parser parser{"test_parser", 5, argv};
     parser.add_option(option_value, 's', "string-option", "this is a string option.");
 
     testing::internal::CaptureStderr();

test/unit/argument_parser/detail/format_help_test.cpp

@@ -197,6 +197,9 @@ TEST(help_page_printing, do_not_print_hidden_options)
 
 TEST(help_page_printing, full_information)
 {
+    int8_t required_option{};
+    int8_t non_list_optional{1};
+
     // Add synopsis, description, short description, positional option, option, flag, and example.
     argument_parser parser6{"test_parser", 2, argv1};
     parser6.info.synopsis.push_back("./some_binary_name synopsis");
@@ -205,10 +208,12 @@ TEST(help_page_printing, full_information)
     parser6.info.description.push_back("description2");
     parser6.info.short_description = "so short";
     parser6.add_option(option_value, 'i', "int", "this is a int option.");
+    parser6.add_option(required_option, 'r', "required-int", "this is another int option.", option_spec::REQUIRED);
     parser6.add_section("Flags");
     parser6.add_subsection("SubFlags");
     parser6.add_line("here come all the flags");
     parser6.add_flag(flag_value, 'f', "flag", "this is a flag.");
+    parser6.add_positional_option(non_list_optional, "this is not a list.");
     parser6.add_positional_option(pos_opt_value, "this is a positional option.");
     parser6.info.examples.push_back("example");
     parser6.info.examples.push_back("example2");
@@ -224,11 +229,15 @@ TEST(help_page_printing, full_information)
                "description\n"
                "description2\n"
                "POSITIONAL ARGUMENTS\n"
-               "ARGUMENT-1 (List of std::string's)\n"
-               "this is a positional option. Default: [].\n" +
+               "ARGUMENT-1 (signed 8 bit integer)\n"
+               "this is not a list.\n"
+               "ARGUMENT-2 (List of std::string's)\n"
+               "this is a positional option. Default: []. \n" +
                basic_options_str +
                "-i, --int (signed 32 bit integer)\n"
                "this is a int option. Default: 5.\n"
+               "-r, --required-int (signed 8 bit integer)\n"
+               "this is another int option.\n"
                "FLAGS\n"
                "SubFlags\n"
                "here come all the flags\n"

test/unit/argument_parser/detail/format_html_test.cpp

@@ -75,7 +75,8 @@ TEST(html_format, full_information_information)
     std::string expected;
     int option_value{5};
     bool flag_value;
-    std::vector<std::string> pos_opt_value{};
+    int8_t non_list_pos_opt_value{1};
+    std::vector<std::string> list_pos_opt_value{};
 
    // Full html help page.
    const char * argv0[] = {"./help_add_test --version-check 0", "--export-help", "html"};
@@ -90,11 +91,11 @@ TEST(html_format, full_information_information)
    parser1.info.long_copyright = "long_copyright";
    parser1.info.citation = "citation";
    parser1.add_option(option_value, 'i', "int", "this is a int option.");
-   parser1.add_option(option_value, 'j', "jint", "this is a int option.");
+   parser1.add_option(option_value, 'j', "jint", "this is a required int option.", option_spec::REQUIRED);
    parser1.add_flag(flag_value, 'f', "flag", "this is a flag.");
    parser1.add_flag(flag_value, 'k', "kflag", "this is a flag.");
-   parser1.add_positional_option(pos_opt_value, "this is a positional option.");
-   parser1.add_positional_option(pos_opt_value, "this is a positional option.");
+   parser1.add_positional_option(non_list_pos_opt_value, "this is a positional option.");
+   parser1.add_positional_option(list_pos_opt_value, "this is a positional option.");
    parser1.info.examples.push_back("example");
    parser1.info.examples.push_back("example2");
    testing::internal::CaptureStdout();
@@ -126,8 +127,8 @@ TEST(html_format, full_information_information)
                           "</p>\n"
                           "<h2>Positional Arguments</h2>\n"
                           "<dl>\n"
-                          "<dt><strong>ARGUMENT-1</strong> (<em>List</em> of <em>std::string</em>'s)</dt>\n"
-                          "<dd>this is a positional option. Default: []. </dd>\n"
+                          "<dt><strong>ARGUMENT-1</strong> (<em>signed 8 bit integer</em>)</dt>\n"
+                          "<dd>this is a positional option. </dd>\n"
                           "<dt><strong>ARGUMENT-2</strong> (<em>List</em> of <em>std::string</em>'s)</dt>\n"
                           "<dd>this is a positional option. Default: []. </dd>\n"
                           "</dl>\n"
@@ -152,7 +153,7 @@ TEST(html_format, full_information_information)
                           "<dt><strong>-i</strong>, <strong>--int</strong> (<em>signed 32 bit integer</em>)</dt>\n"
                           "<dd>this is a int option. Default: 5. </dd>\n"
                           "<dt><strong>-j</strong>, <strong>--jint</strong> (<em>signed 32 bit integer</em>)</dt>\n"
-                          "<dd>this is a int option. Default: 5. </dd>\n"
+                          "<dd>this is a required int option. </dd>\n"
                           "<dt><strong>-f</strong>, <strong>--flag</strong></dt>\n"
                           "<dd>this is a flag.</dd>\n"
                           "<dt><strong>-k</strong>, <strong>--kflag</strong></dt>\n"

test/unit/argument_parser/format_parse_validators_test.cpp

@@ -135,7 +135,7 @@ TEST(validator_test, input_file)
                                "==========="
                                "POSITIONAL ARGUMENTS"
                                "    ARGUMENT-1 (std::filesystem::path)"
-                               "          desc Default: \"\". Valid input file formats: fa, sam, fasta."} +
+                               "          desc Valid input file formats: fa, sam, fasta."} +
                                basic_options_str +
                                basic_version_str;
         EXPECT_TRUE(ranges::equal((my_stdout | std::view::filter(!is_space)), expected | std::view::filter(!is_space)));
@@ -220,7 +220,7 @@ TEST(validator_test, output_file)
                                "==========="
                                "POSITIONAL ARGUMENTS"
                                "    ARGUMENT-1 (std::filesystem::path)"
-                               "          desc Default: \"\". Valid output file formats: fa, sam, fasta."} +
+                               "          desc Valid output file formats: fa, sam, fasta."} +
                                basic_options_str +
                                basic_version_str;
         EXPECT_TRUE(ranges::equal((my_stdout | std::view::filter(!is_space)), expected | std::view::filter(!is_space)));
@@ -274,7 +274,7 @@ TEST(validator_test, input_directory)
                                "==========="
                                "POSITIONAL ARGUMENTS"
                                "    ARGUMENT-1 (std::filesystem::path)"
-                               "          desc Default: \"\". An existing, readable path for the input directory."} +
+                               "          desc An existing, readable path for the input directory."} +
                                basic_options_str +
                                basic_version_str;
 
@@ -319,7 +319,7 @@ TEST(validator_test, output_directory)
                                "==========="
                                "POSITIONAL ARGUMENTS"
                                "    ARGUMENT-1 (std::filesystem::path)"
-                               "          desc Default: \"\". A valid path for the output directory."} +
+                               "          desc A valid path for the output directory."} +
                                basic_options_str +
                                basic_version_str;