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

add read and write file validators #249 #250

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rafiw
Copy link
Contributor

@rafiw rafiw commented Mar 4, 2019

Will close #249.

@rafiw rafiw force-pushed the add_read_write_file branch 2 times, most recently from 3ab8954 to 6f88e6c Compare March 4, 2019 14:00
@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #250 into master will decrease coverage by 0.03%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #250      +/-   ##
==========================================
- Coverage     100%   99.96%   -0.04%     
==========================================
  Files          12       12              
  Lines        2566     2570       +4     
==========================================
+ Hits         2566     2569       +3     
- Misses          0        1       +1
Impacted Files Coverage Δ
include/CLI/Validators.hpp 99.68% <83.33%> (-0.32%) ⬇️

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 be8a08f...6f88e6c. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #250    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          12     12            
  Lines        3397   2786   -611     
======================================
- Hits         3397   2786   -611
Impacted Files Coverage Δ
include/CLI/Validators.hpp 100% <100%> (ø) ⬆️
include/CLI/Formatter.hpp 100% <0%> (ø) ⬆️
include/CLI/Option.hpp 100% <0%> (ø) ⬆️
include/CLI/App.hpp 100% <0%> (ø) ⬆️
include/CLI/StringTools.hpp 100% <0%> (ø) ⬆️
include/CLI/TypeTools.hpp 100% <0%> (ø) ⬆️
include/CLI/Split.hpp 100% <0%> (ø) ⬆️
include/CLI/ConfigFwd.hpp 100% <0%> (ø) ⬆️
include/CLI/Error.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 d9379cc...bef2388. Read the comment docs.

@rafiw
Copy link
Contributor Author

rafiw commented Mar 4, 2019

@henryiii if you agree with this approach i will add tests

@henryiii
Copy link
Collaborator

henryiii commented Mar 4, 2019

CLI::ExistingReadableFile And ...Writable... look like better names I think, but otherwise I like the way you’ve added it! +1

@henryiii
Copy link
Collaborator

Will close #249.

@rafiw rafiw force-pushed the add_read_write_file branch 5 times, most recently from 926fb4f to 8402e14 Compare March 18, 2019 11:58
@henryiii henryiii mentioned this pull request Apr 28, 2019
@rafiw
Copy link
Contributor Author

rafiw commented Jun 12, 2019

@henryiii the permissions in windows is different then the linux once. that why the test fails.
How do you want to approach this?

@cbachhuber
Copy link
Collaborator

What is the plan with this PR? Continue working on it or close it? I can also try to pick up the work

@rafiw
Copy link
Contributor Author

rafiw commented Dec 2, 2019

the issue is with windows since the permissions there is more complicated.
i can put the windows test under ifdef so it will pass.
is this ok?

@phlptp
Copy link
Collaborator

phlptp commented Dec 2, 2019

This would need to be made to work with the changes from #341. For C++17 that enables the use of filesystem so it might be a little easier in C++17 mode on windows now.

@cbachhuber
Copy link
Collaborator

Right, but we would still need to enable file handling (and therefore, this feature) for windows under C++11, correct? Consequently, I would prefer to implement it for Windows & C++11 and not put the test under ifdef. What do you think?

@rafiw
Copy link
Contributor Author

rafiw commented Dec 3, 2019

i will take another look on this and add C++17 support as well

Signed-off-by: Rafi Wiener <rafiw@mellanox.com>
Signed-off-by: Rafi Wiener <rafiwiener@gmail.com>
@phlptp
Copy link
Collaborator

phlptp commented Jan 22, 2020

@henryiii this and a few other issues about validators indicate that some additional validators are of interest. The balance here is that the Validators can take time to compile so for simple applications that don't use them that is a lot of non useful code that needs to compile that must be balanced with the high utility of the additional Validators for those that use them.

What I am wondering if it makes sense to split the Validators into two pieces. The basic definition is required for CLI11, but many of the specific validators and all the machinery that goes with them could be split into a separate file that possibly is even not included in the single file header. Then a separate Validators.hpp would define a library of additional validators, that if someone wanted it they could include that header as well. With the exception of the underlying filesystem operations none of the validators are required by the rest of CLI11. So apart from the basic definition and maybe a couple of the most common validators, the library could be split without issue for 2.0. Then we could get these validators in and some of the other issues without an increasing impact on the compile time.

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.

ExistingFileValidator should also check for read permission
4 participants