Skip to content

Option groups #227

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

Merged
merged 12 commits into from
Mar 1, 2019
Merged

Option groups #227

merged 12 commits into from
Mar 1, 2019

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Feb 19, 2019

This PR add require_options(min,max) functionality to App, the functionality is similar to require_subcommand.

Also create a subclass for Option_group which is currently a limited wrapper around an App, which constructs a nameless subcommand on an app.

the option_group is created through a standalone function so App doesn't have to be aware of the existence of Option_group. This should address #88 as @AlexanderAmelkin, as he is correct that while a nameless subcommand with the require_option will work it does seem kind of a hack. With the option_group we may be able to add some additional better help and possibly some other features related to clusters of options though I am not sure what that might be at this point.

more tests would need to be added if we like this approach.

I think this is an alternative to the radio_id alternative that @AlexanderAmelkin is working on, not sure which is preferred by @henryiii.

@henryiii
Copy link
Collaborator

I like the addition of the required option count, need to think about the named group App subclass idea, it might be better just to integrate the extra functionality into App. Also, you should be able to move that free function into a member, just either predeclare or make it a template function with a wasted automatic template parameter, I use that trick somewhere else in App or Option. Will revisit. A nice example of usage would help decide if it is a good idea to pursue.

@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #227 into master will decrease coverage by 0.28%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
- Coverage     100%   99.71%   -0.29%     
==========================================
  Files          12       12              
  Lines        2057     2089      +32     
==========================================
+ Hits         2057     2083      +26     
- Misses          0        6       +6
Impacted Files Coverage Δ
include/CLI/App.hpp 100% <100%> (ø) ⬆️
include/CLI/Error.hpp 95.58% <50%> (-4.42%) ⬇️

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 5e0bb1c...b07650f. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #227    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          12     12            
  Lines        2212   2402   +190     
======================================
+ Hits         2212   2402   +190
Impacted Files Coverage Δ
include/CLI/Formatter.hpp 100% <100%> (ø) ⬆️
include/CLI/Option.hpp 100% <100%> (ø) ⬆️
include/CLI/App.hpp 100% <100%> (ø) ⬆️
include/CLI/Error.hpp 100% <100%> (ø) ⬆️
include/CLI/StringTools.hpp 100% <0%> (ø) ⬆️

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 4a46081...578b411. Read the comment docs.

@henryiii henryiii mentioned this pull request Feb 20, 2019
@phlptp phlptp force-pushed the option_groups branch 2 times, most recently from 6be3914 to 0335da2 Compare February 22, 2019 23:46
@phlptp phlptp changed the title [WIP] Option groups Option groups Feb 23, 2019
@phlptp phlptp force-pushed the option_groups branch 2 times, most recently from 540b30b to bd05d02 Compare February 23, 2019 12:55
@phlptp
Copy link
Collaborator Author

phlptp commented Feb 23, 2019

I added a ranges example as shown in #88. I think the help needs some work and I need more test cases and docs but I think that example can be run with this PR. The basic idea is to treat option groups the same as options for counting purposes, and then allow options to exclude subcommands, and subcommands to exclude others.

@henryiii
Copy link
Collaborator

I'd like a readme update, even if just so I can get a good feel for what this affects from a user perspective. Also, could this be able to replace the ->group argument (at least for options)? This would add a name to unnamed subcommands (so basically, there would be two ways to make a subcommand, and those would put them into two different lists, "subcommands" and "groups". It might make printing the groups easier as well. For backward compatibility, ->group("name") could add a name group if one does not exist, then move the option there? Just thoughts.

@phlptp
Copy link
Collaborator Author

phlptp commented Feb 27, 2019

I thought about that a little, but if you were to make the current groups use the new option groups it would alter the parse order, which could be important for positional arguments. And I think if you had two lists you lose some of the composability aspects of nameless subcommands, and dealing with switching between them could be a tricky issue. Maybe worth exploring after getting a little experience with how this interacts with everything else. Your ideas on full group objects might come into play as well.

@phlptp
Copy link
Collaborator Author

phlptp commented Feb 27, 2019

I will work on the Readme next, but probably won't be able to finish it until this evening or tomorrow morning.

@henryiii henryiii added this to the v1.8 milestone Feb 28, 2019
add a validation check on min options to make sure it is even possible to succeed.

add some additional tests to cover code paths and potential errors.

add a number of additional tests and checks and fix some issues with the add function in option_groups

clean up example and help formatting

add option_groups example to play with

move create_option_group to a member function using a dummy template

add some optionGroup tests

add min and max options calls and an associated Error call
@phlptp
Copy link
Collaborator Author

phlptp commented Feb 28, 2019

I updated the readMe with the changes, at least a first draft of the update.

@henryiii
Copy link
Collaborator

Is this ready? I think we can merge, possibly iterating before the next release.

@phlptp
Copy link
Collaborator Author

phlptp commented Mar 1, 2019

At your discretion, If you are happy with the readme updates I think it is ready. I am guessing there will need to be a few tweaks as there is more experience with it, but I don't know what those might be at the moment.

@henryiii henryiii merged commit 0631189 into CLIUtils:master Mar 1, 2019
@henryiii
Copy link
Collaborator

henryiii commented Mar 1, 2019

I’m going to consider a few changes/renames in a seperate issue in a few days. Thanks!

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