add 'default_missing_value' configuration option

[rivy]

v3-compatible version of #1468.
Re-opened from #1507 (after upstream branch rename auto-close).

RFC/WIP ~ this PR is a quick proof-of-concept, inviting commentary before being updated and fleshed out (documentation, tests, etc).

For an OPTION taking optional values (ie, .min_values(0).require_equals(true)), currently any time that option is encountered multiple times, occurs is incremented, but no entry is made into the indices and vals arrays for options with no/missing value. "None" would seem like a good entry but the vals array contains just that, values, not Options, so that would break compatibility without any great upside. However, adding a default for missing values can be done without breaking anything else...

This PR adds support to supply a value for the "None"/missing-value case (eg, for "--OPTION") via a default_missing_value() method. Notably, the supplied value to default_missing_value() may differ from the OPTION's default_value().

clap (v3) compiles and tests without error after this patch, but there is no specific testing of the feature as of yet (WIP).

I'm happy to put some more effort towards this (adding tests, documentation, etc) if it is a desired feature addition.

[kbknapp]

As mentioned in the other comments, I'd like to see the most work put towards how we make it clear the difference between default value and default missing value, as well as thinking through all the possible edge cases. Perhaps naming it default_empty_value or such would help.

The one other area of concern is how this interacts with the conditional default values, or should it? I'm perfectly OK saying it doesn't.

[rivy]

toml v0.5.4 is breaking the Travis build ...
Alex doesn't believe that changing the minimum rust version is a "semantic" change?
Maybe lock toml to v0.5.3?

[rivy]

I can easily change the name from default_missing_value to default_empty_value.

I'm concerned a bit about the semantics...

Is --color="" (or --color=) an "empty" value? a null value?

I do think that (1) --color="" (aka --color=) and (2) --color are semantically distinct and should be treated as (1) color with an empty string value (ie, "") and (2) color with no value (which pulls in the default_missing_value in the PR). But I'm not really sure of the best terminology for differentiating the two cases.

[kbknapp]

Is --color="" (or --color=) an "empty" value? a null value?

I do think that (1) --color="" (aka --color=) and (2) --color are semantically distinct and should be treated as (1) color with an empty string value (ie, "") and (2) color with no value (which pulls in the default_missing_value in the PR).

I agree and you make a great point. The best terminology I can think of is --color= or --color="" is explicitly an empty value. Whereas --color is a null value, I like your terminology. So having said that I'd retract my default_empty_value idea, being that --color= and --color="" are actually what clap calls empty values.

[tshepang]

I'd say --color= should mean exactly the same as --color, because a value is not given at all, and that's what @rivy seems to favor also.

[rivy]

@tshepang , no, this PR is actually exactly the opposite. This PR and IMO ...

• --color="" == --color='' == --color=, and means "use the empty string ('') as the value for the color option"
• --color is semantically different, meaning "use the (author specified) default value for the color option"; eg --color would generally be equivalent to --color=always

[tshepang]

Hm, maybe --color= should be disallowed then (runtime error). It's not obvious to me it should be an empty string, instead of null.

[rivy]

@tshepang , most shells parse and return --foo= as the resultant argument for all of --foo=, --foo='', and --foo="". These are supplied, and there is usually no reasonable way to differentiate at the level of the executable. So, generally, --foo= is / has to be interpreted as foo with an empty string argument.

Notably, windows CMD is, as usual, an exception, but most option libraries for windows emulate the *nix standard.

[pksunkara]

@rivy Please rebase and add some tests and docs.

[rivy]

Will do ... in the middle of landing a large PR to coreutils, so it'll have to be next week or so ...

[rivy]

@pksunkara , @kbknapp , the code is now rebased onto 'master' and updated to use the newer clap-v3 internals. I've also added a few tests. I should have some documentation comments done within the next day or so...

Please review and let me know what revisions you'd like and whatever else needs to be done as prep for merge.

[pksunkara]

Looks good. Have a few minor comments. You probably need to make sure cargo fmt doesn't fail.

[pksunkara]

Can you also add a test case which gets empty value for --color= even with default_missing_value specified. (You probably need to say TODO and comment out the test case since empty values don't work yet correctly)

[rivy]

@pksunkara , @kbknapp , @CreepySkeleton , docs added; some polish done based on initial feedback.

[pksunkara]

Looks good.

[pksunkara]

Tests are still failing and you haven't addressed all my comments.

[rivy]

Doc and lint tests are now passing.
All comments addressed, if not completely resolved.

[rivy]

Changes made based on feedback.
I'll squash to 3 commits (add, tests, docs) after initial tests pass.

[pksunkara]

Looks good. I don't have any other comments.

[rivy]

Rebased onto current master, squashed to 3 commits, missing trailing comma added.

Tests are all passing.

[pksunkara]

bors r+

[bors]

Build succeeded:

• CI
• clap-rs.clap
• continuous-integration/travis-ci/push

[twe4ked]

Thanks everyone 🙏