fix(parser): Ban long flags with literals

[rami3l]

Closes #1543.

The objective of this PR is to ban use of literals with long flags (long args without TakesValue), eg. --flag=literal, in order to avoid the current confusion that --flag=false is considered as true.

Ideally, --flag=literal could be re-enabled for certain values of literal, but the choice of such values is left for further discussion (in #1649 for CLI flags, and #2539 for environment variables).

Unresolved Questions

• How should this interact with multiple occurrences?

[epage]

How should this interact with multiple occurrences?

I wish overrides-self was a default :). It'd especially make sense in this case but people can still relatively easily opt-in, if they know how.

[epage]

Looking over #1649, it doesn't look like there was consensus on moving forward with this. It also looked like it was possible to implement this by users in clap today.

I'm particularly concerned about

• User confusion with an argument that can only be used as --arg=value and not also -avalue or --arg value
• The needed special casing of the parser for this
• If we did support this as a --arg value, it'd then need to be opt-in with a flag because conditional number of values can have surprising results.

[rami3l]

@epage As a clap user I truly think that the pseudo-flag pattern solution proposed in #1649 (comment) is acceptable. However, the problem that I really want to solve here is that --flag=false is considered as true, as good library should always minimize its possibility to be misused. Also, I think users need to know how to get the exact behavior that they expect.

I have an idea: to avoid possible argues about true/false against yes/no and 1/0, maybe we can do this in another way by banning --flag=something altogether, and think of a way to guide our users towards the pseudo-flag pattern, also to give our users more control?

How will this decision affect the usage of clap?

[epage]

However, the problem that I really want to solve here is that --flag=false is considered as true

... I didn't want to believe this but I threw in a test real quick

    assert_eq!(
        Opt { alice: true },
        Opt::parse_from(&["test", "--alice=false"])
    );

imo thats bad and seems like a bug in the parser. If there is a val, we shouldn't ignore it but should error if we are processing a flag.

[rami3l]

@pksunkara After some consideration, at this point I'm leaning towards modifying my original code to ban --flag=something altogether. Is this desirable?

@epage What's worse, the same problem exists with env flags, see #2539.

[rami3l]

@epage @pksunkara So I have just banned all usage of flags with a literal, giving a too_many_values error.

The rest of this problem is to somehow make users of this crate more aware of the pseudo-flag approach. Any advice?

If this gets accepted, I will then make another PR on env variables.

[epage]

The rest of this problem is to somehow make users of this crate more aware of the pseudo-flag approach. Any advice?

imo we should have a place to document common arg patterns. Unsure if examples/ is the best place or the website.

[epage]

Could you update your PR to say its resolving #1543 rather than #1649
(turns out we did have an open issue for this)

[rami3l]

@epage @ldm0 I updated the description mentioning both issues: it shouldn't be necessary to make a choice between them. Also, I modified one of the existing examples hoping to make more users aware of the pseudo-flag pattern.

[epage]

I updated the description mentioning both issues: there should be no need to make a choice between them

I take it you are proposing we close #1649 out by rejecting the proposed idea and instead document how to use existing clap APIs?

As I said earlier, I hadn't seen consensus on it. No idea what clap's decision making process is like to say whether documenting is a way of closing #1649 or to just unblock people until a decision is made. This is why I was assuming #1649 was going to be taken out of this PR.

One thing gained by decoupling the decision over #1649 from this PR is it means it doesn't have to be bogged down before merging useful improvements.

[rami3l]

@epage Thanks for clarifying. Sorry, clearly I wasn't very well thought out, since it turns out that soon we might be discussing the very same problem over again with env variables...

[pksunkara]

The example file still has changes.