Skip to content

[FIX] Prohibit long-option value pairs without a separation. #1792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,15 @@ Note that 3.1.0 will be the first API stable release and interfaces in this rele

## Notable Bug-fixes

### Argument Parser

* Long option identifiers and their value must be separated by a space or equal sign `=`.
Handling this restriction resolves the ambiguity if one long option identifier is the prefix of
another ([\#1792](https://github.com/seqan/seqan3/pull/1792)).

Valid short id value pairs: `-iValue`, `-i=Value`, `-i Value`
Valid long id value pairs: `--id=Value`, `--id Value` (prohibited now: `--idValue`)

### I/O

* The `seqan3::field::cigar` was added to the default fields for reading and writing alignment files
Expand Down
34 changes: 26 additions & 8 deletions include/seqan3/argument_parser/detail/format_parse.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -202,16 +202,25 @@ class format_parse : public format_base
}

/*!\brief Finds the position of a short/long identifier in format_parse::argv.
*
* \tparam id_type The identifier type; must be either of type `char` if it denotes a short identifier or
* std::string if it denotes a long identifier.
* \param[in] begin_it The iterator where to start the search of the identifier.
* Note that the end iterator is kept as a member variable.
* \param[in] id The identifier to search for (must not contain dashes).
* \returns An iterator pointing to the first occurrence of the identifier in
* the list pointed to by begin_t. If the list does not contain the
* identifier `id`, the member variable `end_of_options_it` is returned.
*
* Note: The `id` is compared to the prefix of each value in the list, such
* that "-idValue" arguments are correctly identified.
* \details
*
* **Valid short-id value pairs are: `-iValue`, `-i=Value`, or `-i Value`**
* If the `id` passed to this function is of type `char`, it is assumed to be a short identifier.
* The `id` is found by comparing the prefix of every argument in argv to the `id` prepended with a single `-`.
*
* **Valid long id value pairs are: `--id=Value`, `--id Value`**.
* If the `id` passed to this function is of type `std::string`, it is assumed to be a long identifier.
* The `id` is found by comparing every argument in argv to `id` prepended with two dashes (`--`)
* or a prefix of such followed by the equal sign `=`.
*/
template <typename id_type>
std::vector<std::string>::iterator find_option_id(std::vector<std::string>::iterator const begin_it, id_type const & id)
Expand All @@ -220,13 +229,22 @@ class format_parse : public format_base
return end_of_options_it;

return (std::find_if(begin_it, end_of_options_it,
[&] (const std::string & v)
[&] (std::string const & current_arg)
{
size_t id_size{(prepend_dash(id)).size()};
if (v.size() < id_size)
return false; // cannot be the correct identifier
std::string full_id = prepend_dash(id);

return v.substr(0, id_size) == prepend_dash(id); // check if prefix of v is the same
if constexpr (std::same_as<id_type, char>) // short id
{
// check if current_arg starts with "-o", i.e. it correctly identifies all short notations:
// "-ovalue", "-o=value", and "-o value".
return current_arg.substr(0, full_id.size()) == full_id;
}
else
{
// only "--opt Value" or "--opt=Value" are valid
return current_arg.substr(0, full_id.size()) == full_id && // prefix is the same
(current_arg.size() == full_id.size() || current_arg[full_id.size()] == '='); // space or `=`
}
}));
}

Expand Down
53 changes: 43 additions & 10 deletions test/unit/argument_parser/format_parse_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,6 @@ TEST(parse_type_test, add_option_long_id)
EXPECT_TRUE((testing::internal::GetCapturedStderr()).empty());
EXPECT_EQ(option_value, "option_string");

// add with no space
const char * argv2[] = {"./argument_parser_test", "--string-optionoption_string"};
seqan3::argument_parser parser2{"test_parser", 2, argv2, false};
parser2.add_option(option_value, 'S', "string-option", "this is a string option.");

testing::internal::CaptureStderr();
EXPECT_NO_THROW(parser2.parse());
EXPECT_TRUE((testing::internal::GetCapturedStderr()).empty());
EXPECT_EQ(option_value, "option_string");

const char * argv3[] = {"./argument_parser_test", "--string-option=option_string"};
seqan3::argument_parser parser3{"test_parser", 2, argv3, false};
parser3.add_option(option_value, 's', "string-option", "this is a string option.");
Expand Down Expand Up @@ -936,3 +926,46 @@ TEST(parse_test, subcommand_argument_parser_success)
seqan3::argument_parser_error);
}
}

TEST(parse_test, issue1544)
{
{ // wrong separation of long value:
std::string option_value;
const char * argv[] = {"./argument_parser_test", "--foohallo"};
seqan3::argument_parser parser{"test_parser", 2, argv, false};
parser.add_option(option_value, 'f', "foo", "this is a string option.");

EXPECT_THROW(parser.parse(), seqan3::unknown_option);
}

{ // unknown option (`--foo-bar`) that has a prefix of a known option (`--foo`)
std::string option_value;
const char * argv[] = {"./argument_parser_test", "--foo", "hallo", "--foo-bar", "ballo"};
seqan3::argument_parser parser{"test_parser", 5, argv, false};
parser.add_option(option_value, 'f', "foo", "this is a string option.");

EXPECT_THROW(parser.parse(), seqan3::unknown_option);
}

{ // known option (`--foo-bar`) that has a prefix of a unknown option (`--foo`)
std::string option_value;
const char * argv[] = {"./argument_parser_test", "--foo", "hallo", "--foo-bar", "ballo"};
seqan3::argument_parser parser{"test_parser", 5, argv, false};
parser.add_option(option_value, 'f', "foo-bar", "this is a string option.");

EXPECT_THROW(parser.parse(), seqan3::unknown_option);
}

{ // known option (`--foo`) is a prefix of another known option (`--foo-bar`)
std::string foo_option_value;
std::string foobar_option_value;
const char * argv[] = {"./argument_parser_test", "--foo", "hallo", "--foo-bar", "ballo"};
seqan3::argument_parser parser{"test_parser", 5, argv, false};
parser.add_option(foo_option_value, 'f', "foo", "this is a prefix of foobar.");
parser.add_option(foobar_option_value, 'b', "foo-bar", "this has prefix foo.");

EXPECT_NO_THROW(parser.parse());
EXPECT_EQ(foo_option_value, "hallo");
EXPECT_EQ(foobar_option_value, "ballo");
}
}