Skip to content
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

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

Merged
merged 1 commit into from
May 15, 2020

Conversation

smehringer
Copy link
Member

For example, given the long option name "--opt", then "--optValue" was
possible. This caused ambiguities if one long option name is the prefix
of another. We now only allow "--opt Value" and "--opt=value".

Fixes #1544

@smehringer smehringer requested review from a team and eseiler and removed request for a team May 7, 2020 07:42
@eseiler
Copy link
Member

eseiler commented May 7, 2020

And also please add a changelog entry

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #1792 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1792   +/-   ##
=======================================
  Coverage   97.51%   97.51%           
=======================================
  Files         247      247           
  Lines        9401     9401           
=======================================
  Hits         9167     9167           
  Misses        234      234           
Impacted Files Coverage Δ
...ude/seqan3/argument_parser/detail/format_parse.hpp 96.58% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbe8fcb...96b9349. Read the comment docs.

@smehringer smehringer requested a review from eseiler May 7, 2020 09:17
include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@eseiler eseiler requested a review from rrahn May 7, 2020 13:46
@smehringer smehringer force-pushed the argument_parser_option_is_set branch from 9ea67f9 to e3ff068 Compare May 7, 2020 19:46
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! I am wondering if there are some tests missing?! Otherwise a suggestion for a lean lambda function.

include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved
test/unit/argument_parser/format_parse_test.cpp Outdated Show resolved Hide resolved
test/unit/argument_parser/format_parse_test.cpp Outdated Show resolved Hide resolved
test/unit/argument_parser/format_parse_test.cpp Outdated Show resolved Hide resolved
@smehringer smehringer requested a review from rrahn May 12, 2020 09:29
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor things left

include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved
@smehringer smehringer requested a review from rrahn May 13, 2020 08:07
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be merged after rebase and fixing the last column. Thank you

@smehringer smehringer force-pushed the argument_parser_option_is_set branch from d5a0fe0 to e97ad03 Compare May 15, 2020 07:28
For example, given the long option name "--opt", then "--optValue" was
possible. This caused ambiguities if one long option name is the prefix
of another. We now only allow "--opt Value" and "--opt=value".
@smehringer smehringer force-pushed the argument_parser_option_is_set branch from e97ad03 to 96b9349 Compare May 15, 2020 07:29
@smehringer smehringer merged commit 6feff53 into seqan:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Argument Parser throws wrong error
3 participants