add options to handle windows style command line options #187
Conversation
add test cases for windows options and refactor for additional string functions
Codecov Report
@@ Coverage Diff @@
## master #187 +/- ##
==========================================
- Coverage 100% 99.84% -0.16%
==========================================
Files 12 12
Lines 1849 1894 +45
==========================================
+ Hits 1849 1891 +42
- Misses 0 3 +3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #187 +/- ##
==========================================
- Coverage 100% 99.94% -0.06%
==========================================
Files 12 12
Lines 1849 1890 +41
==========================================
+ Hits 1849 1889 +40
- Misses 0 1 +1
Continue to review full report at Codecov.
|
…tion and a few additional test cases to verify documentation
|
I'll try to review on Thursday. Thanks for catching Classifier! |
|
I am not totally clear what to do about the coverage, it seems to be no recording the the closing brace on a lambda function, which seems strange to me. |
|
A little background: I tried a couple different variations the first had a number of different functions to control the parsing. allow_slash_for_short(), allow_slash_for_long(), But that ended up with a lot of conditions and controls in both App and Option. I also looked at allowing the option definition to allow "/option" in addition to the existing one and have the control be through the options. But the parsing is done in the App and that got complex in a hurry. So I went with a single control in App to simplify matters then nothing would need to be changed in the Options. It is possible to allow the use /option=XXX but that is not typical for windows style options, it would require a couple lines in the parse_windows function. So it was left off. My main reason for needing this is even though I prefer the unix style option parsing we have a couple cases where our application is layered with windows specific applications that need to forward command line options, and would require mixing the options style and some odd positional parsing if Windows Style options were not allowed. So allowing that style of options allows everything to be consistent. |
There was a problem hiding this comment.
Overall this looks fantastic. I had a couple of questions. The non-100% coverage issue probably is not the final line of a lambda, but rather a nearby line. For some reason on this and the last PR the coverage line reporting has been off. After it is merged I think we'll see the real line that is causing problems.
| @@ -26,30 +26,6 @@ TEST_F(TApp, ExistingExeCheck) { | |||
| EXPECT_EQ(str3, "\"quoted string\""); | |||
| } | |||
|
|
|||
| TEST_F(TApp, ExistingExeCheckWithSpace) { | |||
There was a problem hiding this comment.
Why is this being removed?
There was a problem hiding this comment.
This was mainly testing the file name separation, which is then redundant with the new Validators test since the function that handles that was moved out of the parse function.
There was a problem hiding this comment.
I think having an integration-style test in addition to the unit-style test is not a bad idea, at least as long as it already exists. I might add it back in.
| /// the string is assumed to contain a file name followed by other arguments | ||
| /// the return value contains is a pair with the first argument containing the program name and the second everything | ||
| /// else | ||
| inline std::pair<std::string, std::string> split_program_name(std::string commandline) { |
There was a problem hiding this comment.
Why is this is Validators.hpp? Shouldn't it be in one of the other headers, like string tools?
There was a problem hiding this comment.
The main reason was it uses the validators to check for an existing file, and if it was included in one of the other tools that header would then have to include the validators header, which I wasn't sure how you felt about.
There was a problem hiding this comment.
Okay, we'll leave it there for now. Maybe should be moved to a new header at some point.
|
Merged, we can look at the coverage after the CI completes on master, and hopefully track down the problem line. |
|
Question: what happens if there's an option named And, note to self, add to changelog. |
|
The long options are checked first so the long option would take precedence in that case |
No, the options are checked one by one in definition order. The first match counts, even if it is |
|
@phlptp, would it make sense to set the default for |
|
The potential drawbacks would be a slight performance degradation to have an additional check for /options and if someone had a relative path using / to start as part of a positional argument that would probably break it if they didn't turn the check off. I wouldn't have any problem with that being the default for my applications but it could cause some unexpected behavior in specific situations, but those would likely be rare. |
If merged this pull request will add an option for CLI11 to handle windows style options
such as /d /D /file:some_file.ext
This is enabled through a new app
function allow_windows_style_options()The modifications include some refactoring of the parse_arg function to take a Classifier enum instead of a boolean and the addition of a parse_windows helper function.
The Classifer enum was renamed to Classifier (not entirely sure if that was a misspelling or on purpose?) If it was on purpose I can change it back.