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

Container options #423

Merged
merged 3 commits into from
Mar 22, 2020
Merged

Container options #423

merged 3 commits into from
Mar 22, 2020

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Feb 5, 2020

If merged this pull request will generalize the types that can be used in add_option to any container.

A container in this case is any type with a value_type member and iterator member, anend() method, a clear() method, and an insert method. This supports all the standard containers, though not stack, or queue since they don't support an insert operation. Should support most other containers as well if they have a similar interface to the standard library ones.

Also support any type with a value_type defined. And supports complex number type (any type with real(), and imag() methods directly in add_option.

One other change is updated google test v1.10.0 to better support templated tests and match the current documentation for google test, so we don't have to figure out where old docs are for getting google test to work.

@phlptp
Copy link
Collaborator Author

phlptp commented Feb 5, 2020

Todo:

  • test with some boost containers
  • make work with something like std::map<std::string, std::pair<int,int>>
  • handle the general wrapper types in the type conversion
  • remove the vector specific operations
  • adjust the priorities for int_constructible and double_constructible classification types
  • update readme
  • update the book chapter on options to match current master
  • get compiling on all tested compilers
  • 100% coverage
  • merge commits and write a summary commit message

@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #423   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        3583   3668   +85     
=====================================
+ Hits         3583   3668   +85
Impacted Files Coverage Δ
include/CLI/Option.hpp 100% <100%> (ø) ⬆️
include/CLI/App.hpp 100% <100%> (ø) ⬆️
include/CLI/StringTools.hpp 100% <100%> (ø) ⬆️
include/CLI/TypeTools.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 967bfe0...063db93. Read the comment docs.

@phlptp phlptp force-pushed the container_options branch 2 times, most recently from a573308 to bfc6a8a Compare February 11, 2020 01:16
@phlptp
Copy link
Collaborator Author

phlptp commented Feb 20, 2020

@henryiii this is getting closer to being ready to go. I am still working on the readme and book probably for a couple days yet.

Had a few questions though. With this PR complex numbers(without requiring the <complex> header be added) are supported directly in add_option. So we can deprecate add_complex, but I wasn't sure if this was something we wanted to do now or in perhaps a later PR.

To handle some options nicely like vector<vector<X>> some sort of user specified segment separator needs to be defined to be able to separate sections on the command line. I was thinking && but I don't have a strong preference.
for examples

--vec 1 2 4 7 && 3 5 --other A

would create a vector with two element vectors one with 4 element and 1 with 2. But I am not sure if && is the best or something else would be better.

@jzakrzewski
Copy link

@phlptp Maybe rather something that does not require quoting in standard shells?

@phlptp
Copy link
Collaborator Author

phlptp commented Feb 20, 2020

@phlptp Maybe rather something that does not require quoting in standard shells?

That is a very good point that I had not been considering thus far.

@phlptp
Copy link
Collaborator Author

phlptp commented Feb 20, 2020

So looking at a list of bash special character
would indicate &~#$*()\{}{};<>/?! would probably not be used since they have different interpretations in shell scripting

That could mean something like %% might work, a single + might work, ++ is used as a subcommand terminator so wouldn't work in this context. # or ## might be possible since this would never begin a line and thus wouldn't be considered a comment line

@phlptp
Copy link
Collaborator Author

phlptp commented Feb 21, 2020

I think I am leaning towards %% as a separator for subvector elements. That doesn't seem to be used anywhere else or have meaning in command line contexts.

@jzakrzewski
Copy link

Bash, and Windows CMD seems to be OK with that.

BTW: # does not have to be at the beginning of the line to start a comment in bash.

@phlptp phlptp changed the title [WIP] Container options Container options Feb 21, 2020
@phlptp phlptp requested a review from henryiii February 21, 2020 15:51
@phlptp phlptp added this to the v2.0 milestone Feb 21, 2020
…nt usage and the modifications to the add_options templates.

add support in add_option for wrapper types, such as std::optional, boost::optional or other types with a value_type trait.  Add support for generalized containers beyond vector,  add support for nested tuples and vectors, and complex numbers directly in add_option.  This includes several new type traits and object categories.

Upgrade the google test version to better support templated tests.

add support for vector argument separator `%%`
@phlptp phlptp force-pushed the container_options branch from 1273c33 to 9c86a5c Compare March 6, 2020 13:31
Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any major issues; we can polish a little as we go if needed, but over all looks very good.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@henryiii henryiii merged commit 27da2f9 into CLIUtils:master Mar 22, 2020
@henryiii henryiii deleted the container_options branch March 22, 2020 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants