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 - PR for initial review only #40

Closed
wants to merge 2 commits into from

Conversation

@eyalroz
Copy link
Contributor

commented Feb 10, 2018

Hello @vprus , @pdimov ,

A year and a half ago I wrote you, @vprus , about my interest in implementing some additional features for program_options. You referred me to the boost mailing list, where I had an exchange with Klemens Morgenstern.

... a long while later, I've gotten around to starting to work on that. Now, the list of all of the features I'm interested in is here, but this PR is about a single feature:

Support multiple alternative long names for some option.

It has another commit, though, unrelated to the feature which is just about avoiding a warning.

I'm asking for a code review of what I've written, to see if it's generally ok with you guys and for some feedback. If you give me the go-ahead I'll add some test cases and make a second PR for you to consider for actual merging.

@jeking3

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2018

I do not see any tests that exercise the new branches in a meaningful way.

@jeking3

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2018

This PR includes a similar fix found in #37.

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2018

@jeking3 : About the fix in #37 - Yes, that's true. I had just fixed it on my fork not noticing other PRs/issues to get rid of the warning. It's not related to the feature itself and if-and-when the feature is deemed fit for merging I can either drop that fix or branch off a fixed version.

About the lack of tests - yes, I have not written tests yet because I first wanted an initial feedback on my approach; ergo this is not a PR to be actually merged. If you (or you and Vladimir? I'm not sure how it works organizationally) like the approach, I will (hopefully) finalize things based on your feedback, write tests, and create PR which would hopefully be ready to merge.

@jeking3

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2018

I maintain other boost libraries but when I saw your email to the mailing list I thought I would look over the backlog here. What exactly is the use case requiring multiple long names on an option?

@pdimov

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2018

This is @vprus 's call. Let's hope that he'll find the time to take a quick look at it.

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2018

@jeking3 :
First - it's not about absolute requirement, it's a convenience feature. After all, you could theoretically only have one long-form option and no short option; nothing absolutely requires short options.

To answer the question though, there are two use cases/aspects of motivation and both of them involve applications with lots and lots of options:

  1. Possibility of having shorthands (e.g. acronyms, but not just) for annoyingly-long options. Example: --frobnicate-special-bars, --fsb.
    Q: "Why not just use a single-character shorthand?"
    A: Not enough single characters; also, difficult to remember if -f is for frobincating bars or for, say, --foo-gets-priority.
    Q: "Why not only use --fsb"?
    A: That means requiring someone to always remember the translation back to the long form when looking a command line somebody else has written; also, it makes it unclear what the acronym stands for in the literal sense
  2. Multiple ways of expressing the same thing, so that the user doesn't always have to remember exactly what phrase the developer chose: "Is it --frobnicate-special-bars or --special-bar-frobnication?" better to just support both.
@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2018

@pdimov : Like I said on the mailing list - I don't want to be disrespectful, but it's a problem if library maintenance is entirely up to one person, on the one hand, but he takes than 4 months to respond (or perhaps over a year and half to consider the oldest PR here), on the other hand.

@jeking3

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2018

Sounds like a reasonable use case to me, thanks for the explanation. You may want to move that into your PR description ( and add tests :) ).

@pdimov

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2018

It is a problem, which is why I said I hoped that he'll chime in, so that we no longer have a problem. :-)

If not, having tests and documentation would be an absolute prerequisite for someone other than the maintainer to merge this.

@vprus

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2018

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2018

Hello @vprus and thanks for chiming in :-)

To address your points:

  • There was a short mailing list discussion in 2016 about this and several other feature proposals I had (thread archived here). The bottom line was me being told:

    You can just fork the library on github, implement the features and create a pull request there.

  • I don't really have a "big context". I've been using program_option in an app of mine, and I since I want to support multiple long options, I need to parse them after-the-fact after collecting the unparsed options. And there are some more features I've been thinking about, but this is perhaps the simplest and most obviously useful.

  • As for "how many users" - I've not done any user/developer surveys about this. If it had been some sort of exotic feature, I would understand you expecting some public "demand"; but in this case - it's basically a a missing feature. Many applications have multiple long-option-names.
    Take gcc for example: You have -traditional and -traditional-cpp; -Wpedantic and -pedantic and so on. Or X (the display server binary) which has -bg and -background-color. If their developers wanted to switch to using program_option, they would want this feature.

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2018

@vprus : You're ignoring this thread again...

Also - are you ok with the approach technically, irrespective of the question of the interest in accepting the PR? If so I'll write tests for it, if not I could conceivably change/fix certain things.

// name as the key, regardless of anything else
if (!m_long_names.empty()) {
const std::string& first_long_name = *m_long_names.begin();
if (first_long_name.find('*') != string::npos)

This comment has been minimized.

Copy link
@vprus

vprus Mar 18, 2018

Collaborator

I am not sure this logic is correct. If we have 3 longs options, with the second one matching the input and having "" at the end, we want to return the actual matched token? This logic will return the matched token only if the first option has trailing ""

This comment has been minimized.

Copy link
@eyalroz

eyalroz Mar 18, 2018

Author Contributor

I don't quite get your example. What do you mean by

"" at the end

? End of what, and what's at that end?

Also, remember that this is key(), not match(); in match() we're obviously more lenient.

This comment has been minimized.

Copy link
@eyalroz

eyalroz Mar 18, 2018

Author Contributor

By the way... is program_options supposed to support an option having an empty long name and no short name? I don't see code in the current set_name() which specifically rules this out (e.g. an assert()).

This comment has been minimized.

Copy link
@vprus

vprus Mar 22, 2018

Collaborator

This code handles a case where long option name has an asterisk at the end (looks like github helpfully removed those asterisks in my original comment. Say, we have long name of foo*, it will match either foo1 or foo2, and the key method will notice his case and use foo1 or foo2 respectively, not foo*. As far as I can see, with your patch, given bar,foo* as long options, and foo1 as token, the method will return bar, not foo1.

This comment has been minimized.

Copy link
@vprus

vprus Mar 22, 2018

Collaborator

Empty long name and no short name is clearly not a useful case, so a diagnostic might be helpful.

@vprus

This comment has been minimized.

Copy link
Collaborator

commented Mar 18, 2018

@eyalroz: I am sorry that my response time is not as good as you (or I, even) would have liked. That said, it's quite common situation in open-source, so please don't take it personally. I had quite a number of patches to other projects that took several pings to get any response.

Speaking of demand/motivation - I am asking because (i) you're the first one to request this functionality (ii) while there are existing tools that have multiple spellings, it's not clear than any new tool would want to do that. In case of gcc, at https://gcc.gnu.org/wiki/DiagnosticsGuidelines they say "-pedantic is an obsolete alias for -Wpedantic" (iii) your earlier examples had fictional option names, so it's hard to judge. In general, if I were creating a CLI today, I would prefer to use a single option name for each option, as aliases might save typing but increase confusion.

As for technical approach, it seems like it should not break existing usage, as long option names with commas would be quite rather. On the other hand, it seems that the default behaviour of option_descriptions formatting would be to show only the first long option, so one easily create CLI where accepted options are not listed in the --help output?

Regards #1: Supporting multiple long names
An initial implementation. It's very unintrusive - no interface changes, and a single addition for obtaining all of the different long names.

Notes:

* 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 passes the existing unit tests, but additional tests for the new functionality have not yet been added

@eyalroz eyalroz force-pushed the eyalroz:develop branch from b6a42d9 to 0487279 Mar 18, 2018

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2018

So, this pull request is now stale (I think) as I've rebased my fork's "develop" branch. Also, I will have at least some work to do following @vprus 's comments and observations of mine following his comments.

But - the general discussion about the merit of this feature is not resolved, so let's continue it here.

  • Reply timing/pings/delays: it's all good by me.... I was just noticing you've been relatively active with this repository over the past couple of weeks. If you'd like to tell me "never ping me more frequently than once in X weeks" I can oblige.

  • Demand for the feature: From looking at the Issues page, you get very very few feature requests. The last one before mine was in 2016. I haven't seen requests on the Boost mailing lists, either. So you could probably say the same about any request at all. I've also mentioned that there don't seem to be statistics or survey data collected about the user base for program_options, nor the capacity to conduct such a survey; so it's essentially impossible to tell how many people would like to have this feature, or would have liked to have it in retrospect. In lieu of that we have the mailing list discussion.

    Would you feel more confident if we brought this up on the mailing list again, to get some "I approve / I disapprove" responses about this feature? I don't mind, but I think its balance of merit vs intrusiveness is so skewed that it's a waste of people's time.

  • Long options containing commas: I don't understand your sentence:

    it seems like it should not break existing usage, as long option names with commas would be quite rather.

    The existing code does not support long option names with commas right now, as the name string is broken up with a find() rather than an rfind(). So the feature doesn't interfere with valid use of the library.

  • Show only first option in description?: Indeed, the way I've coded this feature, only the first option will be shown by os << some_options_description. This intentional choice was the simplest and required no considerations of printing style, wrapping, lengths and so on. I don't have a strong opinion on this choice I've made.

    Note, though, that if a different choice is to be considered, we'd need to think about whether the user wants/needs to see all the options or not, or maybe sometimes s/he wants them all and sometimes not. We'd also need to think about what formatting is appropriate. If we stick with only one option name being printed, I suppose that at the very least some kind of comment and/or documentation note should indicate that the first long name is the canonical name that gets printed in the usage information.

  • The GCC examples: There are more than just -pedantic and -Wpedantic, I gave another one and you can find more aliases.

  • The pros and cons of multiple option names: You've focused on a pair of considerations: Saving typing vs clarity/avoiding confusion. The saving of typing is actually most relevant to the availability of one-letter switches; they save the most typing but are the easiest to confuse, especially since they're specified differently (i.e. -rnone vs --reuse=none). Once you have that, it doesn't seem more confusing to have a third or a fourth alias. So following your argument, you should just remove the short option and have everything very clear-cut....

    Also, saving typing is just one benefit of multiple long names, and probably not the most significant. The most significant IMO is the ability to type in the long option without having to remember the minutiae. If your program only has 5, 10, 20 options, then fine; but if it has 100 or 200 you'll always forget the exact naming of some of the options - even if you've just looked. It helps when you can type in any of --data-all-ones, --all-ones-data, --use-one-for-data, --use-1-for-data, --one-for-data, --fill-data-with-one, --fill-data-with-ones, --use-one-as-data. This example also has bearing on whether all options should always gets printed.

@vprus

This comment has been minimized.

Copy link
Collaborator

commented Mar 24, 2018

Thanks for extended explanation! I still believe that ideally, CLI should have just one naming for any option, but then, I appreciate your argument that having one canonical option, and one abbreviation might be better than having one canonical option and a one-letter abbreviation. Therefore, I'll be find with applying this PR when its technically ready.

One specific point, regarding long options containing commas - what I meant to say, is that indeed, your patch is unlikely to break any existing usage.

@codecov

This comment has been minimized.

Copy link

commented Mar 25, 2018

Codecov Report

Merging #40 into develop will decrease coverage by 0.58%.
The diff coverage is 26.53%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #40      +/-   ##
===========================================
- Coverage    50.29%   49.71%   -0.59%     
===========================================
  Files           23       23              
  Lines         1372     1386      +14     
  Branches       694      708      +14     
===========================================
- Hits           690      689       -1     
- Misses         110      113       +3     
- Partials       572      584      +12
Impacted Files Coverage Δ
...lude/boost/program_options/options_description.hpp 0% <ø> (ø) ⬆️
src/options_description.cpp 47.69% <26.53%> (-3.13%) ⬇️

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 491cb17...dd17186. Read the comment docs.

@eyalroz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2018

Ok, I think we're done with the discussion for now. So - I'm closing this, and will open another PR - hopefully very soon - after I've addressed the (minor) technical points, and added tests.

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