-
Notifications
You must be signed in to change notification settings - Fork 0
Lower target and action deps (3.14.3) #315
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: main
Are you sure you want to change the base?
Changes from all commits
70c1023
3dc71f5
9b8d030
06a3c6f
f6faba3
1a2c686
f875e69
90bcb3a
03f1aec
b7b5589
87de1ae
abb5dea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -137,10 +137,13 @@ fn find_substitution<'a>( | |
| } | ||
|
|
||
| fn substitute(template: &str, ins: &[String], outs: &[String]) -> String { | ||
| let chars: Vec<char> = template.chars().collect(); | ||
| let ins_joined = ins.join(" "); | ||
| let outs_joined = outs.join(" "); | ||
| let mut out = String::with_capacity(template.len()); | ||
| let placeholder_template = template | ||
| .replace("{{ ins }}", &ins_joined) | ||
| .replace("{{ outs }}", &outs_joined); | ||
| let chars: Vec<char> = placeholder_template.chars().collect(); | ||
| let mut out = String::with_capacity(placeholder_template.len()); | ||
|
Comment on lines
+142
to
+146
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win Consider adding a comment explaining the preprocessing stage. The substitution now operates in two stages: first replacing 📝 Suggested inline comment fn substitute(template: &str, ins: &[String], outs: &[String]) -> String {
let ins_joined = ins.join(" ");
let outs_joined = outs.join(" ");
+ // Stage 1: Replace {{ ins }} and {{ outs }} placeholders unconditionally.
+ // Stage 2 (below) performs backtick-aware $in/$out substitution.
let placeholder_template = template
.replace("{{ ins }}", &ins_joined)
.replace("{{ outs }}", &outs_joined);🤖 Prompt for AI Agents |
||
| let mut in_backticks = false; | ||
| let mut i = 0; | ||
| while let Some(&ch) = chars.get(i) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ fn render_rule(rule: &mut crate::ast::Rule, env: &Environment, vars: &Vars) -> R | |
| render_string_or_list(&mut rule.deps, env, vars)?; | ||
| match &mut rule.recipe { | ||
| Recipe::Command { command } => { | ||
| *command = render_str_with(env, command, vars, || "render rule command".into())?; | ||
| *command = render_recipe_str_with(env, command, vars, || "render rule command".into())?; | ||
| } | ||
| Recipe::Script { script } => { | ||
| *script = render_str_with(env, script, vars, || "render rule script".into())?; | ||
|
|
@@ -52,7 +52,7 @@ fn render_target(target: &mut Target, env: &Environment) -> Result<()> { | |
| render_string_or_list(&mut target.order_only_deps, env, &target.vars)?; | ||
| match &mut target.recipe { | ||
| Recipe::Command { command } => { | ||
| *command = render_str_with(env, command, &target.vars, || { | ||
| *command = render_recipe_str_with(env, command, &target.vars, || { | ||
| "render target command".into() | ||
| })?; | ||
| } | ||
|
|
@@ -98,6 +98,22 @@ fn render_str_with( | |
| env.render_str(tpl, ctx).with_context(what) | ||
| } | ||
|
|
||
| fn render_recipe_str_with( | ||
| env: &Environment, | ||
| tpl: &str, | ||
| ctx: &Vars, | ||
| what: impl FnOnce() -> String, | ||
| ) -> Result<String> { | ||
| let mut recipe_ctx = ctx.clone(); | ||
| recipe_ctx | ||
| .entry("ins".into()) | ||
| .or_insert_with(|| ManifestValue::String("{{ ins }}".into())); | ||
|
Comment on lines
+101
to
+110
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Defaulting Because |
||
| recipe_ctx | ||
| .entry("outs".into()) | ||
| .or_insert_with(|| ManifestValue::String("{{ outs }}".into())); | ||
| render_str_with(env, tpl, &recipe_ctx, what) | ||
| } | ||
|
|
||
|
Comment on lines
+101
to
+116
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win Document the helper's ins/outs injection behaviour. Add a doc comment explaining that 📝 Suggested doc comment+/// Render a recipe command template with guaranteed `ins` and `outs` variables.
+///
+/// Clones the provided context and injects default `"{{ ins }}"` and `"{{ outs }}"`
+/// placeholder strings if not already present, ensuring recipe commands can always
+/// reference these variables during MiniJinja rendering. The placeholders are later
+/// substituted with actual paths during command interpolation.
fn render_recipe_str_with(
env: &Environment,
tpl: &str,
ctx: &Vars,
what: impl FnOnce() -> String,
) -> Result<String> {🤖 Prompt for AI Agents |
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
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.
issue (bug_risk): Template-level replacement of
{{ ins }}/{{ outs }}ignores backtick-protected regions, unlike the rest of the interpolation logic.Because the
.replace("{{ ins }}", …).replace("{{ outs }}", …)runs before the backtick-aware scan, these placeholders now get expanded even inside backticks, unlike other substitutions that respect backtick-delimited “no interpolation” regions.To preserve the existing escape semantics, consider handling
{{ ins }}/{{ outs }}inside thefind_substitution/scanning loop (or otherwise applying the same backtick rules to them) rather than doing a global replace first.