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, FEATURE] Fix initialising the version_check_option. Add multi-level argument parsing. #1185

Merged
merged 2 commits into from
Sep 6, 2019

Conversation

smehringer
Copy link
Member

Resolves #1177

@smehringer smehringer requested a review from cpockrandt July 12, 2019 13:14
@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1185      +/-   ##
==========================================
+ Coverage    97.3%   97.31%   +<.01%     
==========================================
  Files         219      219              
  Lines        8803     8832      +29     
==========================================
+ Hits         8566     8595      +29     
  Misses        237      237
Impacted Files Coverage Δ
...lude/seqan3/argument_parser/detail/format_help.hpp 90.45% <100%> (ø) ⬆️
...clude/seqan3/argument_parser/detail/format_man.hpp 97.61% <100%> (+0.05%) ⬆️
...lude/seqan3/argument_parser/detail/format_base.hpp 89.06% <100%> (+0.72%) ⬆️
include/seqan3/argument_parser/argument_parser.hpp 98.54% <100%> (+0.23%) ⬆️
...lude/seqan3/argument_parser/detail/format_html.hpp 92.47% <100%> (+0.08%) ⬆️

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 c376e88...f3a8f3c. Read the comment docs.

@@ -551,18 +553,18 @@ class argument_parser
{
format = detail::format_help{false};
init_standard_options();
return;
special_format_was_set = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that if you pass multiple special formats (e.g., h and hh), it will overwrite it in the next loop iteration. I think we should either print an error message or always take the first one. Currently it would choose the last argument.

@h-2
Copy link
Member

h-2 commented Jul 12, 2019

How does that work with nested argument parsers? For nested arg parser the version check argument should refer to the nested inner arg parser if it comes after the keyowrd. I suspect these changes will break that.

We really need test-cases for nested arg parsers as it is a commonly used design.

@rrahn
Copy link
Contributor

rrahn commented Jul 12, 2019

Sorry to jump into this but what is a nested argument parser? Do we have a snippet or demo/tutorial for this? I can only think of a tool with different configurations who have their own options, e.g.samtools.

@h-2
Copy link
Member

h-2 commented Jul 12, 2019

Sorry to jump into this but what is a nested argument parser?

A program with different sub-programs, like git or lambda (equally famous tools ☝️ ). Each sub-program should have their own help/man-page et cetera...

Do we have a snippet or demo/tutorial for this?

@smehringer said she would write a HowTo at some point (or at least we agreed that we want such a HowTo). Currently we have nothing that tests this, that's why I said new changes threaten to break it.

@rrahn
Copy link
Contributor

rrahn commented Jul 12, 2019

Ok, this is what I had in mind already.

@smehringer smehringer changed the title [FIX] Do always check the whole argv to not miss the version_check_option. [FIX, FEATURE] Fix initialising the version_check_option. Add multi-level argument parsing. Aug 26, 2019
@smehringer smehringer requested a review from eseiler August 26, 2019 07:56
CHANGELOG.md Outdated Show resolved Hide resolved
doc/howto/multi_level_argument_parser/index.md Outdated Show resolved Hide resolved
doc/howto/multi_level_argument_parser/index.md Outdated Show resolved Hide resolved
doc/howto/multi_level_argument_parser/index.md Outdated Show resolved Hide resolved
doc/howto/multi_level_argument_parser/index.md Outdated Show resolved Hide resolved
include/seqan3/argument_parser/detail/format_base.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/detail/format_base.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
@smehringer smehringer requested a review from eseiler August 26, 2019 12:57
@smehringer smehringer force-pushed the argument_parser branch 4 times, most recently from e22099c to 4db19ba Compare August 27, 2019 11:07
@smehringer smehringer requested a review from h-2 August 27, 2019 12:47
@smehringer
Copy link
Member Author

Code coverage complains that format man is not tested 🙄

Copy link
Member

@h-2 h-2 left a comment

Choose a reason for hiding this comment

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

In general: is "multi-level arg parser" the best name? Does this pattern have a name that other projects / arg-parsers use? Maybe "nested" is better?

include/seqan3/argument_parser/argument_parser.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/argument_parser.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/argument_parser.hpp Outdated Show resolved Hide resolved
include/seqan3/argument_parser/argument_parser.hpp Outdated Show resolved Hide resolved
@smehringer smehringer requested a review from h-2 August 29, 2019 14:35
@smehringer smehringer force-pushed the argument_parser branch 2 times, most recently from bd7b893 to 6a5139d Compare September 2, 2019 05:54
@smehringer smehringer requested a review from h-2 September 2, 2019 05:54
@h-2 h-2 removed their request for review September 3, 2019 11:13
@smehringer
Copy link
Member Author

smehringer commented Sep 3, 2019

@h-2 why did you remove your review? 🙀
(I already sais the code coverage fails because of the untested man format, if this is the case)

EDIT: ok ok the ONE comment

@h-2
Copy link
Member

h-2 commented Sep 3, 2019

I remove my review request not because I am pissed, just so that I know which of the items in the list is actually waiting for me and not me waiting on the other person 😉

@smehringer
Copy link
Member Author

I know, I just thought it is because of the code coverage that bites me any time I do something on the argument parser 😇

@smehringer smehringer merged commit 0768d4b into seqan:master Sep 6, 2019
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.

Segmentation fault produced by version check on some machines.
5 participants