[MISC] argument parser: Move get_help_message to file_validator_base. #1691

smehringer · 2020-03-25T14:07:24Z

Just DRY out some code

smehringer · 2020-03-30T05:43:36Z

@marehr polite ping

marehr

I'm not sure if omitting text is better for the user. I liked the output before, what are the benefits for the user?

I'm okay with putting a common function in the base to concat the extensions as a function and use it in the get_help_message.

include/seqan3/argument_parser/validators.hpp

test/unit/argument_parser/format_parse_validators_test.cpp

smehringer · 2020-03-31T06:17:24Z

I'm not sure if omitting text is better for the user. I liked the output before, what are the benefits for the user?

You are right, I didn't notice that there is no mention of the option being an input or output file. I thought there was some more, also regarding the writability stuff. The benefit is, that it poses less of a chance for inconsistencies. I worked on fixing seqan/sharg-parser#77 and adding the compression formats to the help page to each of the two (almost identical...) help page messages called for a DRY version to me.

I'm okay with putting a common function in the base to concat the extensions as a function and use it in the get_help_message.

I'll change it accordingly, and also take a look at the writability

codecov · 2020-03-31T06:58:55Z

Codecov Report

Merging #1691 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    seqan/seqan3#1691      +/-   ##
==========================================
+ Coverage   97.65%   97.68%   +0.02%     
==========================================
  Files         237      237              
  Lines        9050     9059       +9     
==========================================
+ Hits         8838     8849      +11     
+ Misses        212      210       -2

Impacted Files	Coverage Δ
include/seqan3/argument_parser/validators.hpp	`88.88% <100.00%> (+0.13%)`	⬆️
include/seqan3/range/views/kmer_hash.hpp	`100.00% <0.00%> (ø)`
include/seqan3/range/views/take_until.hpp	`98.11% <0.00%> (+3.99%)`	⬆️

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 93de9c7...800af3a. Read the comment docs.

marehr

Looks great :)

Can you assign someone with proper english skills to double check the phrases? 🙈

smehringer · 2020-04-02T06:02:30Z

😄 any suggestion? Usually, I would ask @eseiler...

rrahn · 2020-04-03T14:14:30Z

include/seqan3/argument_parser/validators.hpp

- return detail::to_string("Valid input file formats: [",
- extensions | views::join(std::string{", "}),
- "]");
+ return "The input file must exist and read permissions must be granted. " +


I am not sure. Is that really necessary with the permissions? That kind of clutters the output. It is clear that you need to have permissions. If not you will get an exception saying that you don't have permissions.
I think if the description reads: An input file with a valid extension: [ext1, ext2, ...]
The other thing is An output file with a valid extension: [ext1, ext2, ...]
Then the code is

return valid_extensions_help_page_message("input file");

And the help page message assembles the string.
Then also it checks if valid extensions is empty and omits the part with the valid extensions. Also note, that we should talk about extensions and not formats.

I am not sure. Is that really necessary with the permissions? That kind of clutters the output. It is clear that you need to have permissions.

Well, although "it is obvious that you need them", it is a feature we provide to check for this and I thought this documentation helps to indicate this?

And whatever we decide about the permissions, I think it is important to note that the output file must not exist. This is not the usual behaviour by default as @joergi-w mentioned in his recent issue.

And whatever we decide about the permissions, I think it is important to note that the output file must not exist. This is not the usual behaviour by default as @joergi-w mentioned in his recent issue.

Mhmm, I think it is not necessary but I have no strong feelings. But the other points should be adresed. valid formats ➡️ valid extension and if the extension set is empty this string should not be displayed at all.

…le validators.

smehringer · 2020-04-06T10:54:58Z

I just rebased because there were merge conflicts

rrahn · 2020-04-07T10:06:39Z

include/seqan3/argument_parser/validators.hpp

+ if (extensions.empty())
+ return "";
+ else
+ return detail::to_string("Valid file extensions are: [", extensions | views::join(std::string{", "}), "].");


Suggested change

return detail::to_string("Valid file extensions are: [", extensions | views::join(std::string{", "}), "].");

return detail::to_string("Valid file extensions are: [", extensions | std::views::join(std::string{", "}), "].");

And check that there is no seqan3 views join include.
Regarding detail::to_string. Isn't this part of the dependencies @marehr tried to remove from the argument parser? This could be written as str1 + str2 + str3

The detail::to_string is still present in many parts of the code. He sais this one is an open ToDo and he might reimplement the function so using it here won't make a difference?

This one is tough, because the seqan3::views::join isn't the same as the std::view::join.

I had to warkaround it, but it is okay for me to use seqan3::views::join here. Also keep the detail::to_string.

In this case it is fine as it is.

rrahn · 2020-04-07T10:07:24Z

test/unit/argument_parser/format_parse_validators_test.cpp

+ "Valid file extensions are: [fa, fasta, sam, bam].");
+
+ seqan3::input_file_validator validator2{};
+ EXPECT_EQ(validator2.get_help_page_message(), "The input file must exist and read permissions must be granted. ");


Suggested change

EXPECT_EQ(validator2.get_help_page_message(), "The input file must exist and read permissions must be granted. ");

EXPECT_EQ(validator2.get_help_page_message(), "The input file must exist and read permissions must be granted.");

?

rrahn · 2020-04-07T10:07:48Z

test/unit/argument_parser/format_parse_validators_test.cpp

- "Valid file formats are: [fa, fasta, sam, bam].");
+ seqan3::output_file_validator validator2{};
+ EXPECT_EQ(validator2.get_help_page_message(), "The output file must not exist already and write permissions must "
+ "be granted. ");


Suggested change

"be granted. ");

"be granted.");

?

smehringer · 2020-04-08T08:20:31Z

@rrahn I still have no rights to restart jobs on Jenkins. The error is unrelated. Maybe you can restart Jenkins.

rrahn

lgtm. I'll check jenkins for the rights.

…seqan#1691) * [MISC] argument parser: Refine get_help_page_message for in/output file validators.

smehringer requested a review from marehr March 25, 2020 14:07

smehringer force-pushed the argument_parser_fix branch 2 times, most recently from a989652 to 3cff7d3 Compare March 25, 2020 14:40

marehr reviewed Mar 30, 2020

View reviewed changes

include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved

include/seqan3/argument_parser/validators.hpp Show resolved Hide resolved

test/unit/argument_parser/format_parse_validators_test.cpp Outdated Show resolved Hide resolved

smehringer force-pushed the argument_parser_fix branch from 3cff7d3 to a2a68e7 Compare March 31, 2020 06:58

smehringer requested a review from marehr March 31, 2020 06:59

marehr approved these changes Apr 1, 2020

View reviewed changes

smehringer requested a review from rrahn April 2, 2020 06:02

rrahn requested changes Apr 3, 2020

View reviewed changes

joergi-w mentioned this pull request Apr 5, 2020

Allow to overwrite existing output files. seqan/product_backlog#50

Closed

[MISC] argument parser: Refine get_help_page_message for in/output fi…

ddeebfc

…le validators.

smehringer force-pushed the argument_parser_fix branch from 1ffebe1 to ddeebfc Compare April 6, 2020 10:54

apply requested changes.

d4e90db

smehringer requested a review from rrahn April 7, 2020 07:16

rrahn requested changes Apr 7, 2020

View reviewed changes

apply changes.

c8d47dd

smehringer requested a review from rrahn April 7, 2020 11:01

do use seqan3::views::join

800af3a

rrahn approved these changes Apr 8, 2020

View reviewed changes

smehringer merged commit 1165343 into seqan:master Apr 8, 2020

wvdtoorn pushed a commit to wvdtoorn/seqan3 that referenced this pull request Apr 15, 2020

[MISC] argument parser: Move get_help_message to file_validator_base. (…

f765113

…seqan#1691) * [MISC] argument parser: Refine get_help_page_message for in/output file validators.

smehringer deleted the argument_parser_fix branch May 29, 2020 05:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MISC] argument parser: Move get_help_message to file_validator_base. #1691

[MISC] argument parser: Move get_help_message to file_validator_base. #1691

smehringer commented Mar 25, 2020

smehringer commented Mar 30, 2020

marehr left a comment

smehringer commented Mar 31, 2020

codecov bot commented Mar 31, 2020 •

edited

Loading

marehr left a comment

smehringer commented Apr 2, 2020

rrahn Apr 3, 2020

smehringer Apr 5, 2020

rrahn Apr 6, 2020

smehringer commented Apr 6, 2020

rrahn Apr 7, 2020

smehringer Apr 7, 2020

marehr Apr 7, 2020

rrahn Apr 7, 2020

rrahn Apr 7, 2020

smehringer commented Apr 8, 2020

rrahn left a comment

	return detail::to_string("Valid file extensions are: [", extensions \| views::join(std::string{", "}), "].");
	return detail::to_string("Valid file extensions are: [", extensions \| std::views::join(std::string{", "}), "].");

	EXPECT_EQ(validator2.get_help_page_message(), "The input file must exist and read permissions must be granted. ");
	EXPECT_EQ(validator2.get_help_page_message(), "The input file must exist and read permissions must be granted.");

[MISC] argument parser: Move get_help_message to file_validator_base. #1691

[MISC] argument parser: Move get_help_message to file_validator_base. #1691

Conversation

smehringer commented Mar 25, 2020

smehringer commented Mar 30, 2020

marehr left a comment

Choose a reason for hiding this comment

smehringer commented Mar 31, 2020

codecov bot commented Mar 31, 2020 • edited Loading

Codecov Report

marehr left a comment

Choose a reason for hiding this comment

smehringer commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smehringer commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smehringer commented Apr 8, 2020

rrahn left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 31, 2020 •

edited

Loading