🧹 [code health] Update Bash replace test to use metavariable#131
🧹 [code health] Update Bash replace test to use metavariable#131bashandbone wants to merge 1 commit intomainfrom
Conversation
Updated `test_bash_replace` in `crates/language/src/bash.rs` to use `$A` in the replacer string instead of a hardcoded value, resolving a TODO and improving test robustness. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the Bash replacement test to use a metavariable in the replacement string so the test validates capture-and-substitute behavior instead of relying on a hardcoded replacement value. Sequence diagram for Bash test_bash_replace metavariable capture and replacesequenceDiagram
participant TestRunner
participant test_bash_replace
participant test_replace
participant BashPatternEngine
TestRunner->>test_bash_replace: run()
test_bash_replace->>test_replace: test_replace(echo_123, echo_$A, log_$A)
test_replace->>BashPatternEngine: match("echo 123", pattern: "echo $A")
BashPatternEngine-->>test_replace: captures A = 123
test_replace->>BashPatternEngine: substitute(replacer: "log $A", A = 123)
BashPatternEngine-->>test_replace: "log 123"
test_replace-->>test_bash_replace: ret = "log 123"
test_bash_replace->>test_bash_replace: assert_eq(ret, "log 123")
test_bash_replace-->>TestRunner: test passed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider extending this test (or adding a sibling one) to cover a case where the same metavariable appears multiple times in the replacement string (e.g.,
log $A $A) to better exercise the substitution behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extending this test (or adding a sibling one) to cover a case where the same metavariable appears multiple times in the replacement string (e.g., `log $A $A`) to better exercise the substitution behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Updates the Bash language replacement test to validate metavariable substitution during replacement, aligning it with how other language tests exercise the replace engine.
Changes:
- Replace the hardcoded replacement string (
"log 123") with a metavariable-based replacer ("log $A") intest_bash_replace. - Remove the now-resolved TODO comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🎯 What:
The code health issue addressed is an actionable
TODOincrates/language/src/bash.rsthat suggested using a metavariable in the replacement string oftest_bash_replace.💡 Why:
This improves the test by verifying that the pattern matching engine correctly captures the value into the metavariable and substitutes it back into the replacement string, rather than relying on a hardcoded value. This makes the test a better validator of the underlying replacement logic.
✅ Verification:
The change was verified manually by checking the logic:
"echo 123""echo $A"(captures$A = 123)"log $A""log 123"assert_eq!(ret, "log 123")Automated tests were attempted but could not be completed due to environment-specific dependency issues (offline mode missing
mimallocandbit-setfrom the local Cargo cache). However, the logic is straightforward and consistent with other tests in the repository (e.g., inrust.rs).✨ Result:
The
TODOis resolved, the comment is removed, and the test is now more representative of real-world use cases for the pattern matching engine.PR created automatically by Jules for task 11195642146221711901 started by @bashandbone
Summary by Sourcery
Update the Bash replacement test to validate metavariable substitution rather than a hardcoded replacement string.
Tests:
Chores: