Support for multiple long names#53
Conversation
|
@vprus : It's not clear to me why the Travis & AppVeyor builds fail (and not all of them fail). Looking at the logs, it seems like some kind of library code mismatch (?) Plus, I see that the develop branch has some travis fails before an acceptance of the request, so I'll claim it's Not My Fault (TM). |
|
@vprus : Ping. |
|
@eyalroz: Pong. I'll try to look at this patch soonish, but seems like I should first get CI to become green. |
|
@vprus : Ping. It's been over a month and a half now. |
|
I think I've fixed/suppressed CI issues, will run CI on this branch and get back to you then. |
|
In fact, it probably won't build in CI untill you rebase on top of develop, or merge from develop. Could you do that? |
|
I'll try, but I'm moving out of the Netherlands this week so no guarantees. Also - right now it tells me there are no conflicts with the base branch (= develop); so it might be trivial enough to do it. |
|
@vprus: Ok, so I've rebased. IF you'd like me to do anything else, let me know. |
|
@eyalroz: thanks. It seems that C++03 compilation is now broken, per https://travis-ci.org/boostorg/program_options/jobs/392365094 - mostly due to use of 'auto'. Can you switch to good old for loops? Also, the first warning in the same build log is about unused variable in the code you've added? |
|
The unused variable was "used" in an assert; taken care of. Also - I had mistakenly thought the tests could be C++11. Also fixed. Now I'm just waiting for appveyor... |
Codecov Report
@@ Coverage Diff @@
## develop #53 +/- ##
==========================================
- Coverage 50.29% 49.89% -0.4%
==========================================
Files 23 23
Lines 1372 1385 +13
Branches 694 707 +13
==========================================
+ Hits 690 691 +1
Misses 110 110
- Partials 572 584 +12
Continue to review full report at Codecov.
|
|
@vprus : Ok, so there's not enough test coverage. I'll get around to this later on. |
|
@vprus: The Travis CI builds actually succeeded, except for one which failed internally, trying to install g++7 before building my code. |
An unintrusive implementation - no existing interfaces changed, and a single addition for obtaining all of the different long names. Notes: * Tests added for the new functionality, and existing tests expanded to take it into account. * It is now impossible to specify long names with commas in them (but then, that wasn't properly supported before either, more of an oversight). * The multiple long options are not included in the usage information - just the first one of them is printed
|
This happened again - travis fails before touching my code. @vprus : Will you take my word for it? I've covered the |
|
@eyalroz, yeah, it seems travis is not entirely reliable. I'll ignore that failure. |
|
@vprus : Ok, so - will you merge? If not - anything else you'd like me to do first? |
| } | ||
| catch (multiple_occurrences& e) | ||
| { | ||
| BOOST_CHECK( (e.get_option_name() == "--cfgfile") || (e.get_option_name() == "--configfile")); |
There was a problem hiding this comment.
Is there a typo on 'configfile'? The test has 'config-file', with dash.
There was a problem hiding this comment.
@vprus : Yes, it's a typo. Can you fix it on the development branch of the main repo or would you like a PR?
| BOOST_CHECK( (e.get_option_name() == "--cfgfile") || (e.get_option_name() == "--configfile")); | ||
| BOOST_CHECK( | ||
| (string(e.what()) == "option '--cfgfile' cannot be specified more than once") || | ||
| (string(e.what()) == "option '--configfile' cannot be specified more than once") |
There was a problem hiding this comment.
Same answer as above.
|
Eyal, I've merged this pull request; thanks for you contribution and patience. I had one question, inline, which can be addressed separately. |
|
This was also merged to master for 1.68 and added to release notes for same. Thanks again! |
2 participants
See (prolonged) discussion in the PR-for-initial-review, PR #40.