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

[WIP] Add radio groups #234

Conversation

AlexanderAmelkin
Copy link

@AlexanderAmelkin AlexanderAmelkin commented Feb 20, 2019

Radio groups are named so after HTML radio buttons
(named so after old style car radio station select buttons).

A radio group is a numeric property of an Option.
All options having the same radio_id() become mutually
exclusive and at least one of them is required to be specified.

Example:

CLI::Option *aopt = app.add_option("-a,--aopt", a, "Option A")->radio_id(1);
CLI::Option *bopt = app.add_option("-b,--bopt", b, "Option B")->radio_id(1);
CLI::Option *copt = app.add_flag("-c,--copt", c, "Option C")->radio_id(1);

Resolves #88

Radio groups are named so after HTML radio buttons
(named so after old style car radio station select buttons).

A radio group is a numeric property of an option.
All options having the same radio_id() become mutually
exclusive and at least one of them is required to be specified.

Example:
    CLI::Option *aopt = app.add_option("-a,--aopt", a
                                       "Option A")->radio_id(1);
    CLI::Option *bopt = app.add_option("-b,--bopt", b
                                       "Option B")->radio_id(1);
    CLI::Option *copt = app.add_flag("-c,--copt", c,
                                     "Option C")->radio_id(1);

Resolves CLIUtils#88
@AlexanderAmelkin
Copy link
Author

@henryiii, please tell me what you think about this approach.

@henryiii
Copy link
Collaborator

There are several issues on the surface. I like strings better than numbers; you should have named radio_ids rather than hoping you remember what number they are. Also, minimum and maximum are not available here, it's 1 option in the group only (in keeping with a normal radio button, though). Mostly though, it seems like this should share something programmatically, rather than just by contract with the user and a magic number or string. I'll look at the underlying design a bit more and get back to you again. Also the Option Group idea in #227 might solve some/all of this.

@henryiii
Copy link
Collaborator

The error with list of options is nice, by the way.

@AlexanderAmelkin
Copy link
Author

AlexanderAmelkin commented Feb 21, 2019

I like strings better than numbers

Numbers were selected because I hate strings as a C (not C++) programmer and because:

  • They are faster than strings in comparison operation. CPU can natively compare numbers, not strings. Checking a variable for zero is a single machine instruction unlike checking the monstrous std::string for being empty/unset.
  • It's the programmer who knows how options are grouped and so he can easily assign options to the group by typing just 1 character instead of at least 3 for a string (double quotes count);
  • It's much easier and cleaner to create a single constant with a descriptive name and then use it as an argument to radio_id() than to repeat the same string in each invocation of the function. Having a constant with the string value kind of defeats the purpose of having string IDs;

All in all, I don't see any advantages in using strings unless they are displayed to the user, and since I don't show my radio groups to the user, I decided to go with the numbers.

I didn't understand the comment regarding "the minimum and maximum", by the way.

I agree though that if option groups are implemented as full-scale objects with properties, that could be better.

@phlptp
Copy link
Collaborator

phlptp commented Feb 21, 2019

@AlexanderAmelkin what do you think of the direction taken in #227. Is there something that can be done in the constructors of option_groups that might make it more user friendly and useful?

@henryiii
Copy link
Collaborator

C (not C++)

CLI11 is a C++ library, not C. Though I do know there is a C11. :)

They are faster than strings

Yes. But if there is a measurable impact on the performance of CLI11 due to this one check, CLI11 is in trouble. CLI11 is, by its very nature, manipulating strings. Its input are strings, options are named with strings, etc. And it generally is not manipulating a massive number of strings and options.

typing just 1 character instead of at least 3 for a string

A programmer spends 10% of their time writing code, 20% of their time reading code (often code they wrote), and 70% of there time debugging code. Numbers don't tell me why you grouped something. A name might.

unless they are displayed to the user

Printing helpful help would be nice. A user needs to know what options are required in a group.

It's much easier and cleaner to create a single constant with a descriptive name

Yes, you can't enforce that someone does this, but it would be the best way to use an integer ID value. I notice you did not do this for your example, though.

I didn't understand the comment regarding "the minimum and maximum", by the way.

What if you want to require 2 options from a list of 5? Or exactly 1 option vs. 1 or more options?


My biggest problem with using integers is, though, what happens someone writes a file with some options, and uses option ID 0. Then someone (maybe someone else or a very forgetful person) adds another file with a new option group, but also picks ID 0? This is less likely with strings, and is more understandable if it does happen.


Did you look at #227? Your feedback there would be useful. I'm going to try to work on a full Option group class idea just to see how it compares to the two approaches.

@codecov
Copy link

codecov bot commented Feb 23, 2019

Codecov Report

Merging #234 into master will decrease coverage by 1.29%.
The diff coverage is 82.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #234     +/-   ##
========================================
- Coverage     100%   98.7%   -1.3%     
========================================
  Files          12      12             
  Lines        2057    2090     +33     
========================================
+ Hits         2057    2063      +6     
- Misses          0      27     +27
Impacted Files Coverage Δ
include/CLI/Error.hpp 94.65% <0%> (-5.35%) ⬇️
include/CLI/Option.hpp 100% <100%> (ø) ⬆️
include/CLI/App.hpp 97.74% <100%> (-2.26%) ⬇️

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...76141b0. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Feb 23, 2019

Codecov Report

Merging #234 into master will decrease coverage by 1.29%.
The diff coverage is 82.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #234     +/-   ##
========================================
- Coverage     100%   98.7%   -1.3%     
========================================
  Files          12      12             
  Lines        2057    2090     +33     
========================================
+ Hits         2057    2063      +6     
- Misses          0      27     +27
Impacted Files Coverage Δ
include/CLI/Error.hpp 94.65% <0%> (-5.35%) ⬇️
include/CLI/Option.hpp 100% <100%> (ø) ⬆️
include/CLI/App.hpp 97.74% <100%> (-2.26%) ⬇️

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...76141b0. Read the comment docs.

@henryiii
Copy link
Collaborator

Thanks for this, but I think that the option groups feature solves this. Please feel free to comment on any deficiencies or features you need!

@henryiii henryiii closed this Mar 12, 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.

3 participants