Allow duplicate recipes

[lutostag]

Hey there @casey first off just wanted to thank you for an awesome tool. I personally love using it in all my projects.

This is meant to address the allowing duplicate recipes via a setting. This should help partially resolve the conversation in how to override recipes when using justprep.

I know this conversation was a while ago, and your reasoning may have changed (I really want modules or recipes that can have prereqs from other files), but justprep is the closest we can get to having modular "dependencies" at the moment, and this is sort of the last piece that would help our workflows.

Why I chose to use a setting rather than a cli flag: if there are duplicate recipes in the justfiles, they will not parse, rather than pass around the config flags everywhere it seemed logical to have that be a setting in the file (that if used in an older version will properly barf not knowing about the allow-duplicates setting.

Some tradeoffs made: settings are parsed in order and duplicates are only allowed after the setting is used. I know this breaks the nice unordered property of justfiles generally, but the compromise seemed fair while I was there.

It looks like this now:

set allow-duplicates

# default that might come from `justprep`
foo:
  @echo "basic recipe"

foo:
  @echo "more specific overridden recipe"

$ just
more specific overridden recipe

and without the set allow-duplicates line will error as normal:

error: Recipe `foo` first defined on line 3 is redefined on line 6
  |
6 | @foo:
  |  ^^^

Open to any feedback (both on approach and code) -- or if you want to go a different direction entirely at this point it would be understandable too.

I can also write up the docs as needed, but didn't want to go overboard until got your general approval to refine more

[casey]

Thanks for the PR and the thoughtful comments!

I'm not opposed to a setting. A few comments:

• I think it should be called allow-duplicate-recipes, for clarity. allow-duplicates makes me think that maybe it also allows duplicate variables. (Which might be a desirable thing to add later, since justprep might want to also allow overriding variables.)

• The setting should be able to appear anywhere in the justfile and have the same effect. Otherwise, the docs have to say "All the settings are order independent, except for allow-duplicate-recipes…". I haven't thought about it in depth, but Analyzer::recipes can probably be turned into a Table<'src, Vec<UnresolvedRecipe<'src>>>, and the check for duplicates can be deferred until after all the recipes are analyzed, and depend on whether allow-duplicate-recipes was set or not.

• There should be an integration test that actually runs a recipe that was overridden.

[lutostag]

Thanks for the quick response and feedback!

Went through and made the suggested changes.

Was able to implement the duplicate overriding from within the analyzer relatively cleanly (after reading the settings).

Not sure I added the integration test to the right spot, but put one into tests/misc.rs

Also added quick documentation to the Readme (but unsure if I missed other spots).

Any feedback/suggestions are welcome (even small nits). Thanks!

[casey]

Great! Added a few comments.

[casey]

I try not to add new tests to misc.rs, since it's way too long as is. You can create a new file, tests/allow_duplicate_recipes.rs and put the test in there. Also, I try not to add new macro-based tests, i.e., using the test! macro. Check out one of the struct-based tests that uses Test::new.

[lutostag]

Moved to new file as suggested.

[lutostag]

I think I addressed your comments. As an extra thought however just noting that allow-duplicate-recipes I believe would be the first justfile codification of the word "recipe" to it. It lives throughout the documentation/code and in the unstable json dump area but would not be easy to change in a backwards compatible manner. Looking at #166 particularly. It might actually make sense if we only enable allow-duplicate-recipes setting if we enable the unstable flag in the CLI.

I know that would be more work (and go back to pulling in config options into the analyzer which I tried to avoid... but would keep it behind the feature flag). Also because the only other code effected would be in the json dump code that is also unstable, we wouldn't need to pass the settings around everywhere. (Perhaps the tricky part would be documenting it clearly in the README).

@casey I don't mean to delay a feature I asked for I guess only want to make sure we bolt on new features wisely -- especially with 1.0 around the corner.

I'm fine either merging or doing the work to only allow this when --unstable is passed, up to you entirely but curious on your thoughts.

[casey]

I think I addressed your comments. As an extra thought however just noting that allow-duplicate-recipes I believe would be the first justfile codification of the word "recipe" to it. It lives throughout the documentation/code and in the unstable json dump area but would not be easy to change in a backwards compatible manner. Looking at #166 particularly.

Good thought! I'm actually not too worried. It wouldn't be hard to just soft-deprecate the current setting name, at the same time as we introduce an alias. I'm also on the fence about #166 anyways.

Regarding 1.0, I actually think that anything that we want to change in the future can be introduced in a backwards compatible way with settings. And, if eventually there are a bunch of settings that we want to make the default, we can do so with something like Rust's edition system, where you would put something like set version := 2 at the top of the justfile, which would opt you into a new set of defaults.

So I'm fine including this as is and worrying about it later if we want to change it.

[casey]

There was a clippy error, but it was simple so I went ahead and fixed it. Looks good!

[casey]

I just published 0.11.1, which includes this change.