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

Pre parse callback #251

Merged
merged 5 commits into from
Mar 22, 2019
Merged

Pre parse callback #251

merged 5 commits into from
Mar 22, 2019

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Mar 4, 2019

This PR adds a pre parse callback and clarifies the callback timing of subcommands and option groups.

It will also address issue #166. The basic concept is to allow each app/subcommand/option_group to have two callbacks, one would execute after the first argument of the subcommand/option group is processed. This allows an ability to effect the parsing dynamically, based on the number of arguments passed like Issue #166, or to dynamically enable or disable groups of options based on which options or subcommands were passed, or whatever other program state is appropriate to an application. The other callback executes after parsing is complete. With immediate_callback this is done immediately following the parse. So callbacks from multiple subcommands can be executed in order and executed multiple times. If that flag is not set all callbacks are executed on completion of all parsing in order of creation.

To facilitate adaptive enabling or disabling option_groups some flags were created enabled_by_default and disabled_by_default that reset the disabled_ flag to a defined value on start of parsing.

In the process of this a few issues with subcommand termination were fixed, and the readme updated with a new section on callbacks. Also a subcommand_terminator was added (++) to enable explicitly specifying that a subcommand is finished.

Also added two new examples shapes with repeated subcommands and callbacks
and positional_arity which is Issue #166 coded up.
TODO:

  • add missing tests for code coverage
  • add some helper functions to simplify triggered option groups

I am not sure I like all the names of the flags, so suggestions are welcome.

@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #251 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #251    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          12     12            
  Lines        2566   2718   +152     
======================================
+ Hits         2566   2718   +152
Impacted Files Coverage Δ
include/CLI/Option.hpp 100% <100%> (ø) ⬆️
include/CLI/App.hpp 100% <100%> (ø) ⬆️
include/CLI/StringTools.hpp 100% <100%> (ø) ⬆️

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 be8a08f...684e61c. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #251 into master will decrease coverage by 0.07%.
The diff coverage is 99.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
- Coverage     100%   99.92%   -0.08%     
==========================================
  Files          12       12              
  Lines        2566     2668     +102     
==========================================
+ Hits         2566     2666     +100     
- Misses          0        2       +2
Impacted Files Coverage Δ
include/CLI/Option.hpp 100% <ø> (ø) ⬆️
include/CLI/App.hpp 99.82% <99.32%> (-0.18%) ⬇️

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 be8a08f...6597532. Read the comment docs.

@phlptp phlptp changed the title [WIP] Pre parse callback Pre parse callback Mar 5, 2019
rework return values from _parse_* function to return true if the value was processed false otherwise, this simplified the logic and got rid of the pulling and clearing of the missing fields from option groups.

add TriggerOff and TriggerOn helper functions and some tests for them

add shapes example of multiple callbacks in order.

allow specification of callbacks that get executed immediately on completion of parsing of subcommand

add tests for enabled/disabled by default

add _get_fallthrough_parent.  To get the most appropriate parent to fallthrough to

add enabled and disabled by default functions

add positional_arity example

Add a pre_parse_callback_ for apps.  The Pre parse callback takes an argument for the number of remaining arguments left to process, and will execute prior to parsing for subcommands, and after the first option parse for option_groups.
@phlptp phlptp force-pushed the pre_parse_callback branch from 2115ddc to 2d68c4b Compare March 5, 2019 16:47
@phlptp phlptp force-pushed the pre_parse_callback branch from 5241a51 to 9d421e0 Compare March 6, 2019 05:01
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
include/CLI/App.hpp Outdated Show resolved Hide resolved
include/CLI/App.hpp Outdated Show resolved Hide resolved
include/CLI/App.hpp Outdated Show resolved Hide resolved
include/CLI/App.hpp Outdated Show resolved Hide resolved
include/CLI/App.hpp Outdated Show resolved Hide resolved
include/CLI/App.hpp Show resolved Hide resolved
@henryiii
Copy link
Collaborator

henryiii commented Mar 6, 2019

Names of the flags, etc. seem fine, at least to me.

… strings, some cleanup in the README.md

Apply suggestions from code review

Co-Authored-By: phlptp <top1@llnl.gov>
allow callbacks for option_groups, and allow some other characters as flags
@phlptp phlptp force-pushed the pre_parse_callback branch from 822a383 to 3777889 Compare March 6, 2019 13:44
@phlptp
Copy link
Collaborator Author

phlptp commented Mar 6, 2019

As a note: a number of the issues with callback ordering came up as I am trying to integrate CLI11 into one of our bigger applications. A lot of the capabilities I have worked on over the last few weeks have been driven by the requirements and use cases from those applications, so I think I am nearing the end of integrating it, but we will find out over the next few days as I continue working on that.

@henryiii
Copy link
Collaborator

henryiii commented Mar 6, 2019

We'll wait to do a 1.8 release at least until you have finished integration and verified everything works together. :)

…ype and update some corresponding test cases and documentation

Allow option groups to use ignore_case ignore_underscore for inheritance

Allow option groups to specify allow_extras even if the parent app doesn't in which case extra options flow down into the option_group.
@phlptp phlptp force-pushed the pre_parse_callback branch from 3d62b70 to e5a78ee Compare March 8, 2019 14:21
@phlptp
Copy link
Collaborator Author

phlptp commented Mar 8, 2019

I updated based on #252

@henryiii
Copy link
Collaborator

Is this ready for review/merge?

@henryiii henryiii added this to the v1.8 milestone Mar 12, 2019
@phlptp
Copy link
Collaborator Author

phlptp commented Mar 12, 2019

I have been slowly integrating CLI11 into our bigger applications, and am probably about half way through that effort. I think with what I did yesterday, I need to add a remove_subcommand function to app and add_subcommand(sub *) to the option_group class. But I haven't done that yet or written test cases for them.

So I think you can merge/review as is and I will add the rest and what else I might uncover in another PR or you can wait a few days while I complete the integration.

@phlptp
Copy link
Collaborator Author

phlptp commented Mar 14, 2019

Will close #256

add some test of the remove_excludes functions

add test for Issue CLIUtils#256

add remove_subcommand fail test

add remove_subcommand function and add_subcommand to option_group and some tests associated with them.
@phlptp phlptp force-pushed the pre_parse_callback branch from 06f1914 to 684e61c Compare March 14, 2019 22:48
@phlptp
Copy link
Collaborator Author

phlptp commented Mar 14, 2019

I think this is ready to go, for another look.

@henryiii
Copy link
Collaborator

Okay, will look at it as soon as I can. Traveling back from a conference in Switzerland tomorrow, then to JLab this weekend for another one.

@henryiii henryiii merged commit 49e93ca into CLIUtils:master Mar 22, 2019
@henryiii
Copy link
Collaborator

Looks pretty good, will merge. Standard feedback that I might suggest adjustments later before release.

@phlptp phlptp deleted the pre_parse_callback branch March 23, 2019 03:46
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.

2 participants