-
Notifications
You must be signed in to change notification settings - Fork 82
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
[FEATURE] Adding information only to the advanced help pages is now possible. #1652
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few namespace issues, because we force seqan3::
prefix now, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
b2e8727
to
f6a360d
Compare
Codecov Report
@@ Coverage Diff @@
## master #1652 +/- ##
==========================================
+ Coverage 97.66% 97.67% +0.01%
==========================================
Files 236 237 +1
Lines 8988 9045 +57
==========================================
+ Hits 8778 8835 +57
Misses 210 210
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good to me. Some small changes I'd like to suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have additional minor suggestions.
something broke? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine. Some minor naming suggestions. And please have a look at the failing tests. Many thanks.
* \param[in] spec The option specification. | ||
* \returns `true` if the information annotated with `spec` should be added to the help page, `false` otherwise. | ||
/*!\brief Adds a function object to parser_set_up_calls **if** the annotation in `spec` does not prevent it. | ||
* \param[in] fun The function object that, if added to parser_set_up_calls, prints information to the help page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* \param[in] fun The function object that, if added to parser_set_up_calls, prints information to the help page. | |
* \param[in] print_callback The invocable that, if added to `parser_set_up_calls`, prints information to the help page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name print_callback
does not tell me much. I don't know what you are expressing by callback?
would printer
be fine? :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rrahn ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In computer programming, a callback, also known as a "call-after"[1] function, is any executable code that is passed as an argument to other code; that other code is expected to call back (execute) the argument at a given time.
So that's why callback tells me what it is. Since it is documented as invocable it would be fine for me to call it printer.
Do you want me to squash or do you fix up the commit history? |
da3ba62
to
3ca197e
Compare
I cleaned the commit history |
Fixes #1175
The following functions accept a
seqan3::argument_parser::option_spec::ADVANCED
to control what is displayed on the (advanced) help page:seqan3::argument_parser::add_section
seqan3::argument_parser::add_subsection
seqan3::argument_parser::add_line
seqan3::argument_parser::add_list_item
Note that other
seqan3::argument_parser::option_spec
s likeREQUIRED
are ignored.