Recipe grouping

[neunenak]

Implements recipe grouping feature, see #1776

[neunenak]

I'm currently using the -g flag (for --groups) in this draft PR. This issue: #1648 asks for a global feature, for which it would also be convenient to use the -g flag. I'm not sure which item should get the -g flag, and if so what would be a better alternate flag letter for either.

[casey]

Nice!

You might want to rebase, I just merged a PR which allows overriding the confirm prompt using [confirm("prompt")], so there's some churn in attribute stuff.

I think [group: foo], as you have it, is definitely the nicest syntax. If we have it though, we should also have [confirm: "prompt"] too otherwise users will likely get confused.

[casey]

I think probably the global flag should get -g, since I think it might be more commonly used. The easiest thing to do is probably to not assign short flags to either and wait for someone to complain that one should have a short flag.

[neunenak]

I think probably the global flag should get -g, since I think it might be more commonly used. The easiest thing to do is probably to not assign short flags to either and wait for someone to complain that one should have a short flag.

Makes sense. This is a feature that I personally want and expect to use reasonably frequently, so I'd want to add a short flag. Maybe -r?

[neunenak]

Nice!

You might want to rebase, I just merged a PR which allows overriding the confirm prompt using [confirm("prompt")], so there's some churn in attribute stuff.

I think [group: foo], as you have it, is definitely the nicest syntax. If we have it though, we should also have [confirm: "prompt"] too otherwise users will likely get confused.

Do we want to keep the [confirm(prompt)] syntax around and allow both, or get rid of it? The version you just pushed has the () style, but it would probably be okay to get in a quick change that changes [confirm] to use the :-syntax, push a new 1.23.1 version that changes this before too many people start writing justfiles with the () syntax, then get the rest of groups in later.

[casey]

Makes sense. This is a feature that I personally want and expect to use reasonably frequently, so I'd want to add a short flag. Maybe -r?

Oh, hmmm, I see. --groups will list groups and organize by group when used with --list. So the two uses are just --groups and just --list --groups. What if we made --list organize by group by default? If there are groups in the justfile, I assume that people will want to see them.

Then you wouldn't have to do just --list --groups, which is a super long and annoying invocation. And maybe just --groups is less common, so it doesn't need a short flag?

Do we want to keep the [confirm(prompt)] syntax around and allow both, or get rid of it?

It made it into a release, so we should keep it. But I think the other reason for keeping it is that we might want to have attributes that take multiple arguments. So [attr: arg] is a convenient shorthand when there's a single argument, and [attr(arg)] can be use with one or more arguments.

[neunenak]

Makes sense. This is a feature that I personally want and expect to use reasonably frequently, so I'd want to add a short flag. Maybe -r?

Oh, hmmm, I see. --groups will list groups and organize by group when used with --list. So the two uses are just --groups and just --list --groups. What if we made --list organize by group by default? If there are groups in the justfile, I assume that people will want to see them.

Then you wouldn't have to do just --list --groups, which is a super long and annoying invocation. And maybe just --groups is less common, so it doesn't need a short flag?

Do we want to keep the [confirm(prompt)] syntax around and allow both, or get rid of it?

It made it into a release, so we should keep it. But I think the other reason for keeping it is that we might want to have attributes that take multiple arguments. So [attr: arg] is a convenient shorthand when there's a single argument, and [attr(arg)] can be use with one or more arguments.

I like the idea of listing by groups by default. A justfile without any recipes with a [group] will have the same listing behavior as now so it won't break existing behavior. I agree that listing the group names themselves is uncommon and doesn't need a short option. And yeah I think the convention where [attribute: single-arg] is shorthand for [attribute(single-arg)] that doesn't apply to hypothetical multi-argument attributes is fine.

[neunenak]

Alright, got this working like so:

justfile:

# Comment
[group: alpha]
a:

[group(alpha)]
[group(beta)]
b:

c:

[group:"multi word group"]
d:

[group("alpha")]
e:

[group: "beta"]
[group(alpha)]
f:

just --list output:

Available recipes:
(no group)
    c
[group: alpha]
    a # Comment
    b
    e
    f
[group: beta]
    b
    f
[group: multi word group]
    d

This code deliberately allows you to put a recipe in as many groups as you want, and prints a recipe listing once per group it's in. I think these are good defaults, and if they're not what people want in all cases I think it'd be fine to add additional flags to --list that control this.

[neunenak]

#1344 is also probably taken care of by this PR

[neunenak]

@casey this is ready for review

[jermatic]

when merge?

[casey]

I'm super busy with another project, so just is sadly being neglected right now. Honestly it'll probably take at least a month before I can start grinding down my GitHub notification backlog, which is insanely large at this point.

[epicserve]

@neunenak,

In reading the docs that were added, I'm wondering if you would consider the following output. It's a little more readable to me and is based off of how Django does it.

$ just --list

Available recipes:

[no group]
    email-everyone

[lint]
    cpp-lint
    js-lint
    rust-lint

[rust jobs]
    rust-lint

Instead of:

$ just --list
Available recipes:
(no group)
    email-everyone # Not in a group
[group: lint]
    cpp-lint
    js-lint
    rust-lint
[group: rust jobs]
    rust-lint

[epicserve]

@neunenak,

Thank you for the update! I'm really looking forward to this new feature getting added!

[neunenak]

@neunenak,

Thank you for the update! I'm really looking forward to this new feature getting added!

Thanks! I'm gonna ping @casey about this PR again, hopefully this can get merged pretty quickly.

[casey]

Did a little review, see comments.

[casey]

This will not work in general. Single quoted strings, and triple-delimited strings might have content that can't be contained in a single ". I think the simplest thing is to punt on accepting bare words, and only accept string literals for the time being.

[neunenak]

Makes sense. I modified Group to take a StringLiteral.

[casey]

I'd prefer to leave this syntax for a follow up PR, to keep this one simple.

[neunenak]

Removed the code that did this parsing from the PR

[casey]

These imports can be combined.

[casey]

I started looking at this, and to be honest I don't really like the refactor into multiple functions. I don't really mind long functions, and generally don't like functions which are defined and only used once. I think I prefer being able to read a long linear function, and not indirect through other functions. Also, it makes the review harder, because it's hard to tell what changed and what didn't. Can you inline it back into a single function in Subcommand::list?

[casey]

LGTM!

Some notes:

• On entries of vecs, you can do .or_default().push(x) which saves some characters.
• I had to indent the group names because of a conflict with the way modules are printed. Without indenting the group names, it isn't clear that the modules aren't part of the groups.