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

[MISC] Change value_list_validator construction. #1298

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

smehringer
Copy link
Member

@smehringer smehringer commented Oct 15, 2019

Part of #1196

I noticed that you cannot construct a value_list_validator from a simple range (only from a vector or initialiser list). I added construction from a range, as well as construction from a parameter pack which makes initialiser lists obsolete, but this is an API break. @seqan/core Should I still provide an initialiser list constructor?

Also: where in the changelog did we report API changes?

// What worked before and still works now:
std::vector<int> v{0, 1, 2};
seqan3::value_list_validator{v}; // l/rvalue std::vector 

// What works now that hasn't worked before:
seqan3::value_list_validator{0, 1, 2}; // parameter pack
seqan3::value_list_validator{v | views::take(2)}; // ranges/views

// What worked before but does not work anymore:
seqan3::value_list_validator{{0, 1, 2}}; // initialiser list

@eseiler eseiler changed the title [MISC] Change value_list_valisator construction. [MISC] Change value_list_validator construction. Oct 15, 2019
@h-2
Copy link
Member

h-2 commented Oct 15, 2019

I added construction from a range, as well as construction from a parameter pack which makes initialiser lists obsolete, but this is an API break.

Why is it an API break? If the range interface accepts anything that was accepted before and has the same semantics, the API is preserved. We explicitly state that people may not rely on the exact signature, only on the implied semantics.

@smehringer
Copy link
Member Author

Sorry, I added some examples for clarity. There is a syntax break since I cannot use {{...}} anymore. I thought this was ugly anyway so since we are not API stable yet I would like to remove this version.

@smehringer smehringer requested a review from eseiler October 18, 2019 06:50
CHANGELOG.md Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
test/unit/argument_parser/format_parse_validators_test.cpp Outdated Show resolved Hide resolved
test/unit/argument_parser/format_parse_validators_test.cpp Outdated Show resolved Hide resolved
test/unit/argument_parser/format_parse_validators_test.cpp Outdated Show resolved Hide resolved
@smehringer
Copy link
Member Author

@seqan/core ping

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.

I just looked into it to get an overview of the implied changes. Found some things

include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 22, 2019

Codecov Report

Merging #1298 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1298      +/-   ##
==========================================
+ Coverage   97.41%   97.41%   +<.01%     
==========================================
  Files         216      216              
  Lines        8671     8675       +4     
==========================================
+ Hits         8447     8451       +4     
  Misses        224      224
Impacted Files Coverage Δ
include/seqan3/argument_parser/validators.hpp 88.69% <100%> (+0.27%) ⬆️

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 a3828f1...c19bb3e. Read the comment docs.

@smehringer smehringer requested a review from eseiler October 22, 2019 07:33
@@ -691,16 +692,33 @@ TEST(validator_test, arithmetic_range_validator_error)

TEST(validator_test, value_list_validator_success)
{
// type deduction
// --------------
// all arithmetic types are deduced to double in order to easily allow chaining of arithmetic validators
Copy link
Member

Choose a reason for hiding this comment

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

This explanation could also go into the class docs. I was actually wondering why it's double for all arithmetic types...

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -90,7 +90,7 @@ void initialize_argument_parser(argument_parser & parser, cmd_arguments & args)

//![value_list_validator]
parser.add_option(args.aggregate_by, 'a', "aggregate-by", "Choose your method of aggregation.",
option_spec::DEFAULT, value_list_validator{{"median", "mean"}});
option_spec::DEFAULT, value_list_validator{"median", "mean"});
Copy link
Contributor

Choose a reason for hiding this comment

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

In general these files seem not to follow our style guide which they actually should but that is for another time. It seems we need to go through the tutorials anyway before the release. But maybe you can mark it already as a card so we do not forget about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

which style do you mean specifically?

Copy link
Contributor

Choose a reason for hiding this comment

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

the all-or-nothing rule:
if you break on a line, then put every argument on a separate line

parser.add_option(args.aggregate_by, 
                  'a', 
                  "aggregate-by", 
                  "Choose your method of aggregation.",
                  option_spec::DEFAULT, 
                  value_list_validator{{"median", "mean"}});

include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
test/unit/argument_parser/format_parse_validators_test.cpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
@smehringer smehringer force-pushed the argument_parser branch 2 times, most recently from 23a4527 to 2fcdd2f Compare October 28, 2019 18:33
@rrahn rrahn merged commit 82ab331 into seqan:master Oct 29, 2019
@smehringer smehringer deleted the argument_parser branch November 4, 2020 12:11
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.

5 participants