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

Option delimiter #240

Merged
merged 4 commits into from
Feb 27, 2019
Merged

Option delimiter #240

merged 4 commits into from
Feb 27, 2019

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Feb 26, 2019

This PR addresses an issue I came across working on the Validators. The problem was with vectors and Validators if you wanted a vector with a range of say [0, 20] There are options for Validating individual elements but not after the conversion if you also wanted to use a custom delimiter. Furthermore if you wanted to require 3 elements, that had to be 3 elements before the delimiter was applied.

What is comes down to is the delimiter was being applied too late, and had to be applied in a bunch of places if you wanted to use it.

The solution was to move it as part of the Option class. Any option can now have a delimiter, and the delimiter is applied when the results are loaded into an option. This can allow options that require multiple arguments to accept them as a comma separated list or something along those lines and have that meet the requirement.

One twist on the add_complex is you could add ->delimiter('+') and it would accept argument such as "3+2j" as a single command line argument since the + on +2j is redundant. a negative number would still need to be written like 4+-2j for that to work.

My use case I wanted to able to specify a bunch of flags like --flags=flag1,flag2,flag3,.. and have them checked against a hash table. That would only work if the delimiter was applied before the Validator.

@phlptp
Copy link
Collaborator Author

phlptp commented Feb 26, 2019

@henryiii if you are ok with a few breaking changes regarding the delimiter argument with some of the options, I think you could eliminate a couple of the overloads for options that have the delimiter argument in them. Since there is now a delimiter(char) function on the Option.

@codecov
Copy link

codecov bot commented Feb 26, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #240   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        2212   2212           
=====================================
  Hits         2212   2212
Impacted Files Coverage Δ
include/CLI/Option.hpp 100% <100%> (ø) ⬆️
include/CLI/App.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 7254ea7...a3cf2d8. Read the comment docs.

@henryiii
Copy link
Collaborator

henryiii commented Feb 27, 2019

@henryiii if you are ok with a few breaking changes regarding the delimiter argument with some of the options, I think you could eliminate a couple of the overloads for options that have the delimiter argument in them. Since there is now a delimiter(char) function on the Option.

I think that's better. And API that is not in a released version of CLI11 is fine to change! I strongly prefer Option methods, since they are easy to read, rather than positional arguments that do not describe what they do.

@henryiii
Copy link
Collaborator

Let me know when you are ready for final review & merge. Do you want to drop the extra argument on add_option in this PR or should we do it later?

@henryiii
Copy link
Collaborator

Please update the readme as well (optionally the changelog too, but that's completely optional, since it can cause conflicts when merging multiple PRs, and is pretty easy to do later).

… in App, and update the tests to match updated calls
include/CLI/App.hpp Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@phlptp
Copy link
Collaborator Author

phlptp commented Feb 27, 2019

I think I will let you do the changelog, that is probably worth making sure the style is consistent. I updated the readMe. Let me know if I need to add the defaulted version as a separate overload for vectors. If so it might be desirable to have a test that actually needs that and wouldn't compile without the overload.

@henryiii
Copy link
Collaborator

henryiii commented Feb 27, 2019

If so it might be desirable to have a test that actually needs that and wouldn't compile without the overload.

That would be great, especially since I want to try adding a function to make the overload unneeded. But that's after this PR gets merged.

Edit: Added Issue #241.

@henryiii henryiii merged commit 9f81385 into CLIUtils:master Feb 27, 2019
@henryiii
Copy link
Collaborator

Thanks!

@phlptp phlptp deleted the option_delimiter branch February 27, 2019 17:39
@rafiw
Copy link
Contributor

rafiw commented Feb 28, 2019

Great work

@henryiii henryiii mentioned this pull request Feb 28, 2019
5 tasks
@henryiii
Copy link
Collaborator

henryiii commented Feb 28, 2019

What about 3 + 2j and "3 + 2j"? Along with - not working by itself, I think this wouldn't work very well. (this still is fantastic, I expect using ; and : as separators might be popular)

@henryiii henryiii added this to the v1.8 milestone Feb 28, 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