Skip to content

Conversation

ArchieAtkinson
Copy link

@ArchieAtkinson ArchieAtkinson commented Apr 14, 2025

This PR adds the || syntax to the dependency system, allowing recipes to run on failure.

This is based on discussion in #2583

Example:

# justfile
foo: || bar
  echo foo
   exit 1

bar:
  echo bar

# stdout
foo
bar

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

@casey
Copy link
Owner

casey commented Apr 15, 2025

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!

@casey casey requested a review from Copilot July 3, 2025 22:57
Copy link

@Copilot Copilot AI left a 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 like recovery_index or recover_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 to recovery_offset or recover_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 from subsequents so future maintainers understand its role.
  pub(crate) fn recoveries(&self) -> impl Iterator<Item = &D> {

}
}

// eprintln!("name:{} scope:{}", recipe.name, is_dependency);
Copy link
Preview

Copilot AI Jul 3, 2025

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.

Copy link
Owner

@casey casey left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants