-
Notifications
You must be signed in to change notification settings - Fork 592
Recovery Dependencies - Execute Receipe(s) on Recipe Failure #2709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_error
is ambiguous; consider renaming it to something likerecovery_index
orrecover_start
for better clarity.
pub(crate) if_error: usize,
src/parser.rs:959
- [nitpick] The variable
if_error
does not clearly convey its purpose; renaming torecovery_offset
orrecover_start
would improve readability.
let if_error = dependencies.len();
src/recipe.rs:510
- [nitpick] Add a doc comment describing the
recoveries
method and how it differs fromsubsequents
so 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove or replace this commented-out debug statement with proper logging or delete it to keep the code clean.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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