Make methods from ArgMatches panic on unknown argument

[CreepySkeleton]

Closes #604

This PR also introduces a little breaking change: you now need to explicitly define ArgGroups, you can't just Arg::group("group") and have the group created behind the scenes". I STRONGLY believe that this worth it because it turns out it's really easy to mistype the arg name or confuse it with short/long flag names. Some of our tests are broken because of it, and one of our examples was teaching incorrect behavior. The check introduced makes sure this kind of errors won't happen again.

[pksunkara]

I am not exactly convinced about the breaking change regarding groups here.

it's really easy to mistype the arg name or confuse it with short/long flag names.

That is why we have debug assertions checking all those cases and warning accordingly.

Can we try to separate out the breaking change from this PR and see what happens?

[CreepySkeleton]

You don't get it.

// Notice that we don't explicitly create the group
// It's just conjured from the thin air behind the scenes
app.
    .arg(Arg::new("arg").group("try"))
    .arg(Arg::new("opt").group("try"))

Now guess what happens if you mistype the name of the group, like "fry"? The wrong group is to be made up in background! No errors, just a footgun. If you aren't convinced of the impact, look at the diff, doc comment on top of the multiple_values method. Our own documentation failed for the trap.

I think this kind of "feature" should be cut out no matter what. It doesn't serve any purpose except saving devs from typing extra .group(ArgGroup::new("try")) symbols.

[pksunkara]

I understand what you are saying but I am still not convinced. We should prioritise ease-of-use. If they make a typo for a group name, it's their problem.

[CreepySkeleton]

Ease of use is composed of many components, the most important being:

• Ease to write code (what you're arguing for). I acknowledge the importance of this.
• Ease to read code for other people and yourself after a year. Code is read much more then written.
• Ease to detect and fix mistakes if whenever you make them.

The feature in question is optimizing for the first bullet point by sacrificing the last one because the error is just about undiscoverable until you actually run and thoroughly test your program. Without it, the first point is only very slightly damaged, if at all, because these group() invocations is something you do very rarely (how many groups do you have? Let's say at most ten for the most complex programs). Do you really think readability or ease of writing is noticeably affected by an extra dozen of lines of code per program? Nah.

Incidentally, this goes against clap's own convention when you apply it to args. Does required_if("incorrect_name") create a default incorrect_name arg? Than why should .group("incorrect_name") do the same?

Ad absurdum: what does rustc do when it encounters an unknown name? Does it create a hidden variable and initialize it with Default::default()? I think we both agree that would be lunacy and the "unknown name" error is a good thing.

[pksunkara]

bors r+

[bors]

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[pksunkara]

bors r+

[bors]

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[pksunkara]

bors r+

[bors]

Build succeeded:

• CI