Conversation
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)
📝 WalkthroughWalkthroughThis PR refactors pattern stdlib functions to use centralized type-extraction helpers. A new ChangesPattern Helper Consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 pattern stdlib to use the shared argument-validation helpers, adds a new expect_pattern extractor, and removes a redundant Arc<str> allocation in the pattern split fast path. It fits into the broader stdlib cleanup work by standardizing native-function validation and aligning pattern helpers with the rest of the repository’s stdlib conventions.
Changes:
- Added
expect_patterninsrc/stdlib/helpers.rsand reused shared validation helpers in pattern natives. - Simplified
pattern_matches,pattern_find,pattern_find_all,pattern_replace, andpattern_splitargument/type extraction. - Updated pattern stdlib tests to assert the new standardized error-message format.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/stdlib/pattern.rs |
Refactors pattern native functions to use shared helpers and optimizes Arc<str> reuse in no-op paths. |
src/stdlib/helpers.rs |
Adds the new shared expect_pattern extractor for Value::Pattern. |
src/stdlib/pattern_test.rs |
Updates unit tests to match the new helper-driven validation messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Native function: pattern_matches(text, pattern) -> boolean | ||
| /// Tests if text matches the given compiled pattern | ||
| pub fn pattern_matches_native(args: Vec<Value>) -> Result<Value, RuntimeError> { | ||
| if args.len() != 2 { | ||
| return Err(RuntimeError::new( | ||
| "pattern_matches requires exactly 2 arguments (text, pattern)".to_string(), | ||
| 0, | ||
| 0, | ||
| )); | ||
| } | ||
|
|
||
| let text_str = match &args[0] { | ||
| Value::Text(s) => s.as_ref(), | ||
| _ => { | ||
| return Err(RuntimeError::new( | ||
| "First argument to pattern_matches must be text".to_string(), | ||
| 0, | ||
| 0, | ||
| )); | ||
| } | ||
| }; | ||
| check_arg_count("pattern_matches", &args, 2)?; |
| /// Native function: pattern_find(text, pattern) -> object or null | ||
| /// Finds the first match of pattern in text | ||
| pub fn pattern_find_native(args: Vec<Value>) -> Result<Value, RuntimeError> { | ||
| if args.len() != 2 { | ||
| return Err(RuntimeError::new( | ||
| "pattern_find requires exactly 2 arguments (text, pattern)".to_string(), | ||
| 0, | ||
| 0, | ||
| )); | ||
| } | ||
| check_arg_count("pattern_find", &args, 2)?; |
| .to_string() | ||
| .contains("Expected text, got Number") | ||
| ); | ||
| } |
| let _replacement = | ||
| expect_text(&args[2]).map_err(|e| RuntimeError::new(e.message, line, column))?; |
| /// Extracts a Pattern value from a WFL Value, returning it as a reference-counted CompiledPattern. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `value` - The WFL Value to extract from | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// Returns an `Rc<CompiledPattern>` clone (incrementing the reference count) if the value | ||
| /// is a Pattern variant. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns `RuntimeError` if the value is not a Pattern. |
| expect_pattern, | ||
| Pattern, | ||
| Rc<CompiledPattern>, | ||
| "a Pattern", | ||
| |p: &Rc<CompiledPattern>| Rc::clone(p) |
| check_arg_count("pattern_find_all", &args, 2)?; | ||
| let text = expect_text(&args[0])?; | ||
| let compiled_pattern = expect_pattern(&args[1])?; |
| /// Native function: pattern_find_all(text, pattern) -> list | ||
| /// Finds all matches of pattern in text | ||
| pub fn pattern_find_all_native(args: Vec<Value>) -> Result<Value, RuntimeError> { | ||
| if args.len() != 2 { | ||
| return Err(RuntimeError::new( | ||
| "pattern_find_all requires exactly 2 arguments (text, pattern)".to_string(), | ||
| 0, | ||
| 0, | ||
| )); | ||
| } | ||
|
|
||
| let text_str = match &args[0] { | ||
| Value::Text(s) => s.as_ref(), | ||
| _ => { | ||
| return Err(RuntimeError::new( | ||
| "First argument to pattern_find_all must be text".to_string(), | ||
| 0, | ||
| 0, | ||
| )); | ||
| } | ||
| }; | ||
| check_arg_count("pattern_find_all", &args, 2)?; |
Summary of Changes
src/stdlib/pattern.rsfile contained highly redundant argument validation and type extraction logic using manualmatchblocks forValue::TextandValue::Pattern. Additionally, there was a performance debt wherepattern_splitunnecessarily allocated a newArc<str>from a text slice that it already owned anArcreference to (Arc::from(text)instead ofArc::clone(&text)).Arc::fromconversion eliminates a redundant heap allocation, reducing overhead during pattern splitting operations and satisfyingclippy::useless_conversionwarnings.expect_patterntosrc/stdlib/helpers.rsusinggenerate_expect!. Refactored all native pattern functions (pattern_matches_native,pattern_find_native,pattern_find_all_native,native_pattern_replace, andnative_pattern_split) to usecheck_arg_count,expect_text, andexpect_pattern. Fixed the performance debt by updatingArc::fromtoArc::clonein thepattern_splitfast-path and updated the standard library tests to expect the new error message formats.Verification Checklist
cargo fmtexecuted and passed.cargo clippyreturned no warnings or errors.cargo testsuites passed (100% success rate).PR created automatically by Jules for task 13034455029881876816 started by @logbie
Summary by CodeRabbit
Bug Fixes
Refactor