Allow parameters with * to accept zero or more arguments

[rjsberry]

Closes #633

Hi @casey, I'd appreciate any comments you have to make on my implementation.

I personally think we should try to come up with a name to separate variadics prefixed by * and +. I think this will make both the code and documentation clearer to understand. You'll see that I've referred to them as "zero or more" and "one or more" as I couldn't think of a better way to summarize the cardinalities!

[casey]

Overall this looks great! Mostly very tiny issues, see review comments.

I know that the test will pass, but I think it would be good to make sure that you can't have both + and *, like so:

foo *a +b:
  echo {{a}} {{b}}

Referring to them as "star variadic" and "plus variadic" is the best I can come up with. It will probably be easy for users to remember, since "+" and "*" have established meanings in regexes, so calling it "plus" and "star" lets users easily connect to those established meanings.

I can't think of a reason to not allow star variadics with default arguments, although I might be missing something. For consistency, I would allow it, so that users who don't read the docs and just use * because it's familiar don't need to learn about + if they want default args.

[casey]

For consistency I think * variadic arguments should also allow default values. Or is there something I'm not thinking of that makes this problematic?

[rjsberry]

Nothing that makes it problematic per se, but if a star variadic has a default it can never be called with zero instantiations of the parameter. To me that sort of defeats the purpose of something that is "zero or more", but I can certainly appreciate that for consistency it might be better to allow this.

[casey]

Oh yeah, I also wanted to add some color about some of the things I try to keep in mind when thinking about a change like this. I don't think they're issues in this case, but they're good to keep in mind.

The first is that there are a relatively small number of ascii symbols, making each one kind of precious. Whenever a new one is added to Just, I try to think about whether or not it's the best use of that character, or some other use or meaning might be more appropriate. In this case, I think * is totally appropriate, since it has a well established meaning that users are likely to understand.

The second is to try to think about potential conflicts or ambiguities in the grammar, current or future. There is actually an ambiguity in the grammar with +, since it can be ambiguous whether or not a + is introducing a new, variadic argument, or if it's a concatenation:

foo a=b + c

There are other reasons why this doesn't work, namely that you can't have a required argument after one with a default, but ignoring that for a second, it's ambiguous whether the user means foo a=b +c or foo a=b+c.

With * variadics, I don't think there are any conflicts now, but it's good to think about if there could be conflicts in the future. I think it's okay though, since any other reasonable usage of * that I can think of wouldn't introduce a conflict.

[casey]

Oh, almost forgot, * should be added to GRAMMAR.md, probably like so:

recipe        : '@'? NAME parameter* variadic? ':' dependency* body?

parameter     : NAME
              | NAME '=' value

variadic      : '*' parameter
              | '+' parameter

[casey]

Also, unless it's part of your workflow, you don't need to force-push the branch. You can just push new diffs, and I'll squash and merge at the end. That makes it easier for me to see what's changed.

[casey]

I keep forgetting things >_< It would also be good to add a test in parser.rs, see the recipe_variadic test. It should test that it produces a + instead of a * when + is given.

[rjsberry]

Wow, I really appreciate all the comments. Thanks!

It's really useful to have some insight into some of your thoughts behind introducing changes like this to Just.

[casey]

You bet!

I think allowing default values for * args is probably desirable. I agree that it's technically redundant, but I expect that users will be surprised that they can't give a * a default value.

Also, I just thought of this, I think that prefix should return Option<&'static str>. It simplifies a couple of the call sites, since they can do:

if let Some(prefix) = kind.prefix() {
  // use prefix
}

Instead of having to call is_variadic. Also, I noticed that the "" prefix for singular arguments is never actually used.

[rjsberry]

It's funny you mention that about prefix, I actually thought of doing that but reasoned that the explicit variadic check was a little clearer. This was before the method was renamed to prefix though which definitely helps with clarity.

[casey]

Merged! Thanks for doing this, it's a great addition!