feat: allow groups on submodules

[jmwoliver]

Hello! I've been working on a fix for #2243.

I added the whole attributes object instead of just passing around the group like how the doc is. My rationale for this was to make it easier to have access to anything stored in the attributes in the future, let me know if that is the wrong path though.

This is my first time contributing to just, I'm open to any feedback!

[casey]

Nice! I think I would avoid putting storing all attributes on a Justfile, for a couple reasons. One abstract reason is that I try not to make changes in anticipation future needs. There's a good chance those future needs may manifest, or they might manifest differently than expected. And one concrete reasons is that attributes serialize in a somewhat odd way, so they will be difficult to parse using jq, if you use --dump --dump-format json to dump a justfile, whereas a groups: Vec<String> field will be easy to parse, so I think I would separate out groups and just store the group names on Justfile.

[jmwoliver]

@casey Thanks for the feedback! Great points, I'll get this updated to just pass in the group names instead.

[jmwoliver]

@casey I switched to passing around groups: Vec<String>, hopefully this looks better!

[jmwoliver]

Fixed linter errors and re-pushed.

[casey]

Nice, left some initial comments! To check if the PR will pass CI, run just ci, which runs clippy and errors on warnings.

[casey]

We should have a test for the ordering of submodules in groups if --unsorted is used.

My first thought was that submodules should be ordered based on their position in the justfile. However, I'm now not sure if that's correct. I think it's natural to have submodules all declared together at the top or bottom of a justfile, and natural to list submodules at the end of each group.

So maybe my comment above is off, and we should, instead of using submodule.name.unwrap().offset in the ordering key, use usize::MAX instead.

[jmwoliver]

Hm, I tested setting submodule.name.unwrap().offset vs. usize::MAX, and both have the same behavior. Both implementations output the submodules at the end of a group because of how we output recipes and then submodules in subcommand.rs:

for (i, group) in ordered.into_iter().enumerate() {
  for recipe in recipes {...}
  for (i, submodule) in submodules.iter().enumerate() {...}
}

If we wanted the ability to order based on the submodule's position in the justfile, then we would need to interleave the recipes and submodules. But since it sounds like we want them at the end of a group, I'll go with usize::MAX. Let me know if you disagree.

[jmwoliver]

I added tests for --unsorted with and without --list-submodules. I ended up going with submodule.name.unwrap().offset because I think that better guarantees the submodules are ordered how they appear in the justfile at the bottom of a group.

[jmwoliver]

@casey Thanks for the all the great feedback and the just ci tip. I've updated everything with your recommendation, let me know how it looks!

[casey]

Nice, looks good to me!