Make inputs required (show help if not provided)

[mre]

Fixes #327
@fauust fyi

[MichaIng]

Probably keep that [--]? I mean the hint is in mentioned below as well, but I remember I've overseen it in one place and required a few attempts before I figured out that -- was possible and required to have the last arguments count as inputs instead of option arguments 😄.

[mre]

Yeah I noticed that as well, but I ran some tests and couldn't find a case where the -- was still required. Structopt seems to remove the [--] from the usage section if it's not strictly necessary. 🤷‍♀️
Any way you can reproduce the old failing behavior with this branch? If so, we need to find out how to enforce the old syntax in the usage info (which is auto-generated and tested by a doc-test, so we can't just manually edit it). If not, we might want to remove the hint below. (Which I count as a win. 😄)

BTW this also still works, so it's not a breaking change unless we can find another issue:

lychee -- README.md

[MichaIng]

Ah, even when using an option with multiple value support, like:

lychee --exclude 'this' 'that' 'next' README.md

How does it know where the excludes end and where the inputs start?

[mre]

Yeah that doesn't work:

lychee --exclude this that next README.md
error: The following required arguments were not provided:
    <inputs>...

[mre]

This is the code for the [--] part:
https://github.com/clap-rs/clap/blob/88dec14775fc94170b79c38d88da5385c5b9b6e8/src/output/usage.rs#L84-L106

[mre]

So the easiest thing was to add last = true. No idea if that's how it's meant to be done. 😆

[MichaIng]

So now the last input argument is automatically used as input in such case (or is it that inputs need to be the last arguments now)? Still good to keep the hint visible as also this may happen:

lychee --exclude 'this' 'that' index.html README.md

[mre]

I understood it as "inputs need to be the last arguments now".

[MichaIng]

Okay, then I don't understand how last = true helps in this context 😄. However, I think its okay when -- is required in some cases. Better to force users to be explicit than being implicit and guess wrong about what was meant. The only other alternative I see would be to allow only single options values, and e.g. have multiple excludes/schemes/etc separated by ,. But especially with excludes which may contain literal , themselves it is a bit difficult and would be a breaking change.

... although also multiple excludes (same with includes) can be merged into a single regex (we do it like that):

lychee --exclude '^https://(example.org|another.com|localhost)'

and multiple schemes can easily be comma-separated. Only with headers it would be complicated to achieve.

Very interesting that the clap-rs crate adds [--] when there are options which allow multiple values (the code look quite clear about that) and even that there are several which do, it didn't add it in our case?

[lebensterben]

@mre

BTW this also still works, so it's not a breaking change unless we can find another issue:

@MichaIng

Very interesting that the clap-rs crate adds [--] when there are options which allow multiple values (the code look quite clear about that) and even that there are several which do, it didn't add it in our case?

It's a convention that -- signals the end of CLI options/flags. Anything after that is parsed as positional arguments.

But it's still helpful to remind users of --. Suppose a filename starts with -, one way to handle it correctly is to use lychee -- -FILENAME.

Also, although there's no need to manually make input as the last argument (actually it's the first positional argument). it's easier for users to learn the syntax in case they don't know the -- convention.

[mre]

This doesn't work yet.

     Running `target/debug/lychee --exclude-all-private fixtures/TEST_ALL_PRIVATE.md`
error: Found argument 'fixtures/TEST_ALL_PRIVATE.md' which wasn't expected, or isn't valid in this context

USAGE:
    lychee --exclude-all-private

The above case worked before (without --).

[lebensterben]

This doesn't work yet.

     Running `target/debug/lychee --exclude-all-private fixtures/TEST_ALL_PRIVATE.md`
error: Found argument 'fixtures/TEST_ALL_PRIVATE.md' which wasn't expected, or isn't valid in this context

USAGE:
    lychee --exclude-all-private

The above case worked before (without --).

removing , last = true fixes the problem.

See https://docs.rs/clap/2.33.3/clap/struct.Arg.html#method.last

Specifies that this arg is the last, or final, positional argument (i.e. has the highest index) and is only able to be accessed via the -- syntax

[MichaIng]

and is only able to be accessed via the -- syntax

Ah that was the part we didn't expect 👍. Then I wonder why -- was not added automatically without braces (mandatory) 🤔. However, so required = true alone should be fine, leaving everything else untouched.

[mre]

Then I wonder why -- was not added automatically without braces (mandatory)

last = true does not imply required = true. The dashes are only mandatory if the last positional argument is indeed required. If it isn't, the dashes can be left off because if you omit the last positional argument then the other arguments will still work without the dashes and don't need any separation from the optional positional argument.
Long explanation, but that's what's going on in my opinion.

[MichaIng]

Now [--] was again removed despite required = true being set.

Checking code: https://github.com/clap-rs/clap/blob/88dec14775fc94170b79c38d88da5385c5b9b6e8/src/output/usage.rs#L84-L106

I think the problem is .get_positionals().any(|p| !p.is_set(ArgSettings::Required)) which means "there is any positional argument which is not required". So [--] is not added in our case as the only positional arguments "inputs" are now required. This doesn't fit to what the comment states:

places a '--' in the usage string if there are args and options supporting multiple values

IMHO [--] makes sense in any case where options with multiple values are present, regardless whether the positional ARGS are mandatory or not and whether they are required to be last or only shown last in the usage string, as -- may be required in any such case, as shown in the two examples above:

# This fails not despite but because of "required = true" without "--":
lychee --exclude 'this' 'that' 'next' README.md
# And even with "last = true" it fails to do what you want as long as multiple ARGS are allowed:
lychee --exclude 'this' 'that' index.html README.md

So I'd re-add [--] again and report this upstream 🙂.

[mre]

I agree with your conclusion. The current implementation in clap is not technically incorrect however, so they might decline a change. The -- isn't strictly required in all cases and the help message looks a little cleaner. Maybe that's what they were aiming for. On the other hand if a user doesn't know about the -- trick, they might get stuck in such a situation anyway. Then again even if the -- was mentioned, they probably wouldn't know what that would be used for if they never came across that convention before. ¯\_(ツ)_/¯
I for one consider it good form to add the [--] in this case. Just don't want to add last = true and make the -- mandatory, because it's cumbersome for simple use-cases.

> lychee README.md
error: Found argument 'README.md' which wasn't expected, or isn't valid in this context

USAGE:
    lychee [FLAGS] [OPTIONS] [--] <inputs>...

This would be lychee -- README.md with last=true, which is inconvenient.

But probably we should just ask them about their opinion. I don't consider this a blocker, though.
@MichaIng?

[MichaIng]

Isn't it an option to add it manually for now? Anyway not a blocker. I just remember that I exactly got stuck, even that I knew/used the trick before and even that it was in the usage string 😄: #113
So it is still mentioned in the inputs section.

[mre]

Yeah we could overwrite the description, but then we'd run risk of forgetting to keep it up-to-date in the future. And incorrect information is worse than no information, so I'd like fix that upstream if possible.
Since it's not a blocker, I went ahead and merged this.
Thanks for the input @fauust, @MichaIng, and @lebensterben.