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

Type size refactor #325

Merged
merged 33 commits into from
Nov 9, 2019
Merged

Type size refactor #325

merged 33 commits into from
Nov 9, 2019

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Sep 30, 2019

If merged this PR will refactor the type_size and expected size and the multiOptionPolicy so they work independently and allow more flexible types in add_option.

Also rework add_complex to actually allow things like --comp 4+2i or --comp -1.7-5j in addition to the current pair of doubles support.

@phlptp
Copy link
Collaborator Author

phlptp commented Sep 30, 2019

This is getting closer but wanted to see the coverage and let people start taking a look.

@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #325    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          12     12            
  Lines        3186   3331   +145     
======================================
+ Hits         3186   3331   +145
Impacted Files Coverage Δ
include/CLI/StringTools.hpp 100% <ø> (ø) ⬆️
include/CLI/Validators.hpp 100% <100%> (ø) ⬆️
include/CLI/Config.hpp 100% <100%> (ø) ⬆️
include/CLI/Option.hpp 100% <100%> (ø) ⬆️
include/CLI/App.hpp 100% <100%> (ø) ⬆️
include/CLI/Formatter.hpp 100% <100%> (ø) ⬆️
include/CLI/TypeTools.hpp 100% <100%> (ø) ⬆️
include/CLI/Error.hpp 100% <100%> (ø) ⬆️
... and 1 more

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 41d3c96...1ab5e5b. Read the comment docs.

@henryiii
Copy link
Collaborator

Please rebase and force-with-lease, please. CI should be fixed. You might need to run pre-commit, but unlikely.

@phlptp
Copy link
Collaborator Author

phlptp commented Oct 25, 2019

clang tidy is not exactly intuitive what was going on there. It is a nice check but it would be helpful if the actual warning was displayed along with the code, not sure if that is possible or this was a diff of some kind.

@henryiii
Copy link
Collaborator

I think clang-tidy should print info messages while it builds, then the check that fails is just a diff. So if you look back in the build log, you should see a reason, I think.

@phlptp
Copy link
Collaborator Author

phlptp commented Oct 25, 2019

Yeah I guess it does, I was just looking at the step that showed the failure didn't occur to me to check the build for the warning. I should look into how you did that, might be useful to some of our other projects.

@phlptp phlptp marked this pull request as ready for review October 26, 2019 03:41
@phlptp
Copy link
Collaborator Author

phlptp commented Oct 26, 2019

The basics of this PR are working through the details of isolating type size and expected size and allowing them to have a min and max independently. In doing so this addresses #330, #322, and #296, and gets at #257, though I think with a little more work it might be possible to handle that without specifying the template arguments on add_option. That will have to wait for a while though.

There is also probably better treatment of #305 in this PR.

There was some slightly odd behavior left in place to make sure it was consistent with previous behavior, that might be able to be taken out in a 2.0 release if desired.

@henryiii henryiii merged commit 418b717 into CLIUtils:master Nov 9, 2019
@henryiii henryiii deleted the type_size_refactor branch November 9, 2019 19:06
@jzakrzewski jzakrzewski mentioned this pull request Dec 16, 2019
@henryiii henryiii added this to the v1.9 milestone Dec 31, 2019
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.

2 participants