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

Support for multiple long names #53

Merged
merged 1 commit into from Jun 27, 2018

Conversation

@eyalroz
Copy link
Contributor

commented Apr 21, 2018

See (prolonged) discussion in the PR-for-initial-review, PR #40.

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

@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).

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2018

@vprus : Ping.

@vprus

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2018

@eyalroz: Pong. I'll try to look at this patch soonish, but seems like I should first get CI to become green.

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2018

@vprus : So, there's PR #55 which seems to be relevant to doing that. Also - I don't suppose I could be of help?

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2018

@vprus : Ping. It's been over a month and a half now.

@vprus

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2018

I think I've fixed/suppressed CI issues, will run CI on this branch and get back to you then.

@vprus

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2018

In fact, it probably won't build in CI untill you rebase on top of develop, or merge from develop. Could you do that?

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

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.

@eyalroz eyalroz force-pushed the eyalroz:develop branch from ae76850 to 791d597 Jun 14, 2018

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

@vprus: Ok, so I've rebased. IF you'd like me to do anything else, let me know.

@vprus

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2018

@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?

@eyalroz eyalroz force-pushed the eyalroz:develop branch 2 times, most recently from e493a62 to 553c779 Jun 15, 2018

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

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

This comment has been minimized.

Copy link

commented Jun 15, 2018

Codecov Report

Merging #53 into develop will decrease coverage by 0.39%.
The diff coverage is 31.25%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
...lude/boost/program_options/options_description.hpp 0% <ø> (ø) ⬆️
src/options_description.cpp 48.64% <31.25%> (-2.17%) ⬇️

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 4b81bea...0404a0d. Read the comment docs.

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

@vprus : Ok, so there's not enough test coverage. I'll get around to this later on.

@eyalroz eyalroz force-pushed the eyalroz:develop branch from 553c779 to a8485ca Jun 15, 2018

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

@vprus: The Travis CI builds actually succeeded, except for one which failed internally, trying to install g++7 before building my code.

Fixes #1: Support for multiple long names per option
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

@eyalroz eyalroz force-pushed the eyalroz:develop branch from a8485ca to 0404a0d Jun 15, 2018

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

This happened again - travis fails before touching my code. @vprus : Will you take my word for it? I've covered the long_names() method which codecov complained about.

@vprus

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2018

@eyalroz, yeah, it seems travis is not entirely reliable. I'll ignore that failure.

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2018

@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"));

This comment has been minimized.

Copy link
@vprus

vprus Jun 27, 2018

Collaborator

Is there a typo on 'configfile'? The test has 'config-file', with dash.

This comment has been minimized.

Copy link
@eyalroz

eyalroz Jun 29, 2018

Author Contributor

@vprus : Yes, it's a typo. Can you fix it on the development branch of the main repo or would you like a PR?

This comment has been minimized.

Copy link
@vprus

vprus Jul 6, 2018

Collaborator

Done, thanks.

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")

This comment has been minimized.

Copy link
@vprus

vprus Jun 27, 2018

Collaborator

Same question as above.

This comment has been minimized.

Copy link
@eyalroz

eyalroz Jul 6, 2018

Author Contributor

Same answer as above.

@vprus vprus merged commit 589d558 into boostorg:develop Jun 27, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@vprus

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2018

Eyal,

I've merged this pull request; thanks for you contribution and patience. I had one question, inline, which can be addressed separately.

@vprus

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2018

This was also merged to master for 1.68 and added to release notes for same. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.