Fix parameter substitution when both {p} and {p:format} appear#292
Merged
daniel-thom merged 2 commits intomainfrom Apr 28, 2026
Merged
Fix parameter substitution when both {p} and {p:format} appear#292daniel-thom merged 2 commits intomainfrom
daniel-thom merged 2 commits intomainfrom
Conversation
substitute_parameters and substitute_parameters_regex used `if let
Some(...) { ... continue; }` for the {p:format} branch, which meant
that whenever a parameter had a format-spec occurrence, the simple
`{p}` occurrences were never substituted. Templates mixing both forms
(e.g., `run.sh {w} 2024-{w:02d}-01`) silently kept the bare `{w}`
literal in the rendered output.
Replace the conditional with a `while let Some(...)` loop and remove
the `continue` so both forms are handled, and so multiple distinct
format specs for the same parameter (e.g., `{i:02d}` and `{i:03d}`)
are all substituted.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes template parameter expansion so {p} placeholders are still substituted even when {p:format} is present, and ensures multiple distinct format specs for the same parameter are handled.
Changes:
- Update
substitute_parameters/substitute_parameters_regexto replace all{param:format}occurrences before replacing{param}. - Add regression tests covering mixed
{p}+{p:format}, multiple distinct specs, and regex-escaped substitutions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…plate syntax
The previous fix used `result.replace(&full_pattern, &replacement)` inside
a `while let Some(...) = result.find(&pattern)` loop. For String parameters,
`ParameterValue::format(Some(_))` ignores the format spec and returns the
raw value, so a String param whose value contains `{param_name:...}` would
cause `replace` to be a no-op and `find` to keep returning the same index
— spinning forever.
Switch to a cursor-based scan with `replace_range`, so the loop only ever
inspects bytes that have not yet been substituted. This guarantees
termination regardless of what a parameter value contains. Added a
regression test that hangs without the fix.
Reported by Copilot on PR #292.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a bug in template parameter expansion where the presence of a {p:format} occurrence prevented substitution of bare {p} occurrences for the same parameter. This improves correctness for mixed-form templates and ensures multiple distinct format specs for the same parameter are all substituted.
Changes:
- Reworked
{param:format}substitution to iterate through all occurrences using a cursor-based scan (avoids re-scanning substituted text and supports multiple format specs). - Ensured
{param}substitution always runs even when{param:format}was present. - Added regression tests covering mixed
{p}+{p:format}, multiple format specs, termination when values contain template-like text, and the regex-escaping variant.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
substitute_parameters and substitute_parameters_regex used
if let Some(...) { ... continue; }for the {p:format} branch, which meant that whenever a parameter had a format-spec occurrence, the simple{p}occurrences were never substituted. Templates mixing both forms (e.g.,run.sh {w} 2024-{w:02d}-01) silently kept the bare{w}literal in the rendered output.Replace the conditional with a
while let Some(...)loop and remove thecontinueso both forms are handled, and so multiple distinct format specs for the same parameter (e.g.,{i:02d}and{i:03d}) are all substituted.