[JULES] Refactor Pattern Standard Library Functions to Eliminate Redundancy - #483
[JULES] Refactor Pattern Standard Library Functions to Eliminate Redundancy#483logbie wants to merge 1 commit into
Conversation
The Issue: The pattern functions (pattern_matches_native, pattern_find_native, pattern_find_all_native, native_pattern_replace, native_pattern_split) contained duplicated code for matching on Value enum variants to extract strings and patterns, and used manual arg length checks. Furthermore, they had unnecessary to_string() allocations for error messages. The Rational: Eliminating boilerplate through expect helpers improves maintainability and removes performance debt introduced by redundant format!/to_string() calls. The Solution: Implemented a new expect_pattern helper using the generate_expect! macro. Refactored all pattern native functions to use expect_text, expect_pattern, and check_arg_count instead of manual pattern matching and string cloning. Co-authored-by: logbie <1138960+logbie@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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughA new ChangesPattern Validation Helper Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR refactors the stdlib pattern native functions to use the shared stdlib argument/type extraction helpers, reducing repetitive Value matching and standardizing error messages across the pattern API.
Changes:
- Added a new
expect_patternextractor to the stdlib helpers (macro-generated viagenerate_expect!). - Refactored
pattern_matches_native,pattern_find_native,pattern_find_all_native,native_pattern_replace, andnative_pattern_splitto usecheck_arg_count,expect_text, andexpect_pattern. - Updated unit tests to assert against the new unified “expects N arguments” / “Expected …, got …” error message formats, and replaced unnecessary
Arc::from(...)withArc::clone(...)where applicable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/stdlib/pattern.rs |
Swaps manual argument/type matching for shared helpers; reduces allocation by cloning extracted Arc values where possible. |
src/stdlib/helpers.rs |
Introduces expect_pattern extractor to standardize pattern value extraction and error messages. |
src/stdlib/pattern_test.rs |
Updates assertions to match the standardized error message phrasing produced by the shared helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| check_arg_count("pattern_replace", &args, 3) | ||
| .map_err(|e| RuntimeError::new(e.message, line, column))?; | ||
|
|
||
| let _replacement = match &args[2] { | ||
| Value::Text(t) => t.as_ref(), | ||
| _ => { | ||
| return Err(RuntimeError::new( | ||
| "Third argument must be text".to_string(), | ||
| line, | ||
| column, | ||
| )); | ||
| } | ||
| }; | ||
| let text = expect_text(&args[0]).map_err(|e| RuntimeError::new(e.message, line, column))?; | ||
| let _pattern = | ||
| expect_pattern(&args[1]).map_err(|e| RuntimeError::new(e.message, line, column))?; | ||
| let _replacement = | ||
| expect_text(&args[2]).map_err(|e| RuntimeError::new(e.message, line, column))?; |
| check_arg_count("pattern_split", &args, 2) | ||
| .map_err(|e| RuntimeError::new(e.message, line, column))?; | ||
|
|
||
| let text = match &args[0] { | ||
| Value::Text(t) => t.as_ref(), | ||
| _ => { | ||
| return Err(RuntimeError::new( | ||
| "First argument must be text".to_string(), | ||
| line, | ||
| column, | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| let pattern = match &args[1] { | ||
| Value::Pattern(p) => p, | ||
| _ => { | ||
| return Err(RuntimeError::new( | ||
| "Second argument must be a pattern".to_string(), | ||
| line, | ||
| column, | ||
| )); | ||
| } | ||
| }; | ||
| let text_arc = expect_text(&args[0]).map_err(|e| RuntimeError::new(e.message, line, column))?; | ||
| let text = text_arc.as_ref(); | ||
| let pattern = | ||
| expect_pattern(&args[1]).map_err(|e| RuntimeError::new(e.message, line, column))?; |
| generate_expect!( | ||
| /// Extracts a compiled pattern value from a WFL Value, returning it as a reference-counted CompiledPattern. | ||
| expect_pattern, | ||
| Pattern, | ||
| Rc<CompiledPattern>, | ||
| "a pattern", | ||
| |p: &Rc<CompiledPattern>| Rc::clone(p) | ||
| ); |
Summary of Changes
pattern_matches_native,pattern_find_native,pattern_find_all_native,native_pattern_replace, andnative_pattern_split) insrc/stdlib/pattern.rscontained highly redundant code for matching onValueenums to extract texts and compiled patterns. They also manually checked argument lengths and constructed repetitive error strings using.to_string(), generating unnecessary allocations.patternmodule. It also removes the performance debt and technical debt associated with the repetitive allocations and manual boilerplate checks, replacing them with the project's standard extraction mechanisms.expect_pattern, tosrc/stdlib/helpers.rsalongside the other standardized type extractors. Refactored all functions insrc/stdlib/pattern.rsto usecheck_arg_count,expect_text, andexpect_pattern. Furthermore, innative_pattern_splitandnative_pattern_replace,Arc::fromwas replaced with the zero-allocationArc::cloneusing the extracted arcs. Tests insrc/stdlib/pattern_test.rswere updated to assert against the new, unified error messages.Verification Checklist
cargo fmtexecuted and passed.cargo clippy --all-targets -- -D warningsreturned no warnings or errors.cargo testsuites passed (100% success rate).PR created automatically by Jules for task 201104489420881395 started by @logbie
Summary by CodeRabbit