Recovery Dependencies - Execute Receipe(s) on Recipe Failure #2709
Recovery Dependencies - Execute Receipe(s) on Recipe Failure #2709ArchieAtkinson wants to merge 6 commits intocasey:masterfrom
Conversation
b9f1471 to
b41c9e0
Compare
|
Thank you for the PR! Just a warning, I'm super backlogged, so I'm not sure when I'll be able to review this. Just letting you know so that you don't interpret no response as the PR not being welcome or interesting! |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new || recovery dependency syntax so that specified recipes run only when a prior recipe fails.
- Adds parsing support for the
||token and recovery dependency list - Extends the AST and runtime to track and execute recovery recipes on failure
- Updates tests and formatting to cover the new recovery behavior
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/parser.rs | Capture recovery dependencies after ` |
| src/unresolved_recipe.rs | Store the if_error index in the unresolved recipe |
| src/recipe.rs | Add if_error field, recoveries iterator, and format ` |
| src/justfile.rs | Run recovery recipes on failure in execution loop |
| src/lexer.rs | Recognize ` |
| tests/recoveries.rs | New integration tests for recovery dependencies |
| tests/no_dependencies.rs | Test for --no-deps skipping recovery dependencies |
| tests/misc.rs | Update error message expectations for ` |
| tests/json.rs | Include new if_error in JSON serialization tests |
| tests/format.rs | Add formatting test for a recipe with ` |
| tests/lib.rs | Register recoveries module in test harness |
Comments suppressed due to low confidence (3)
src/recipe.rs:28
- [nitpick] The field name
if_erroris ambiguous; consider renaming it to something likerecovery_indexorrecover_startfor better clarity.
pub(crate) if_error: usize,
src/parser.rs:959
- [nitpick] The variable
if_errordoes not clearly convey its purpose; renaming torecovery_offsetorrecover_startwould improve readability.
let if_error = dependencies.len();
src/recipe.rs:510
- [nitpick] Add a doc comment describing the
recoveriesmethod and how it differs fromsubsequentsso future maintainers understand its role.
pub(crate) fn recoveries(&self) -> impl Iterator<Item = &D> {
| } | ||
| } | ||
|
|
||
| // eprintln!("name:{} scope:{}", recipe.name, is_dependency); |
There was a problem hiding this comment.
Remove or replace this commented-out debug statement with proper logging or delete it to keep the code clean.
casey
left a comment
There was a problem hiding this comment.
Thanks for your patience!
Please ignore the copilot review. I was curious what it would say.
Correct me if I'm wrong, but I believe there is a problem here: || recipes will only run if the recipe that failed was giving on the command line, but not if the recipe was a dependency of another recipe.
So if we have:
a: || recovery
exit 1
b: || recovery
exit 1
c: b
recovery:Then just a will run recovery, but just c will not run recovery.
I think the fix is to put the recovery code into Justfile::run_recipe, where it will execute for all recipes, not just top-level command-line invocations.
This PR adds the
||syntax to the dependency system, allowing recipes to run on failure.This is based on discussion in #2583
Example:
Please let me know if any additional work needs to be done or changes to the current behaviour.
One potential improvement that could be considered is passing the status code from the failing recipe into the recovery recipes