[Refactor] Deduplicate pattern argument checking logic - #334
Closed
logbie wants to merge 4 commits into
Closed
Conversation
Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
Contributor
|
👋 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. |
Contributor
|
Claude finished @logbie's task —— View job Code Review Complete ✅This is a well-executed refactoring that successfully deduplicates argument checking logic. Here's my comprehensive review: ✅ StrengthsCode Quality & Best Practices:
Type Safety & Error Handling:
|
Contributor
📝 WalkthroughWalkthroughCentralized argument validation for pattern stdlib functions; added Changes
Sequence Diagram(s)sequenceDiagram
participant Interpreter as Interpreter
participant Stdlib as Stdlib::pattern
participant Engine as PatternEngine
participant Runtime as RuntimeError
Interpreter->>Stdlib: call native_pattern_replace(args)
Stdlib->>Stdlib: check_arg_count / expect_text / expect_pattern
Stdlib->>Engine: compiled_pattern.matches/find/replace(...)
alt match succeeds
Engine-->>Stdlib: match results
Stdlib-->>Interpreter: Ok(Value)
else engine error
Engine-->>Stdlib: Err(lib_error)
Stdlib-->>Interpreter: Err(lib_error) %% returns error without line/col
Interpreter->>Runtime: map_err -> RuntimeError::with_kind(line,column, ...)
Runtime-->>Interpreter: RuntimeError (propagated)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors stdlib pattern functions to reuse shared argument-count/type-extraction helpers, and updates interpreter wiring + tests to match standardized errors.
Changes:
- Replaced per-function argument checking in
pattern.rswithcheck_arg_count,expect_text, and newexpect_pattern. - Standardized
native_pattern_replace/native_pattern_splitto take onlyargs, with interpreter-side error remapping to attach source locations. - Updated pattern stdlib tests to match the new helper-driven error messages.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/stdlib/pattern.rs | Deduplicates arg validation/type extraction via helpers; standardizes replace/split signatures. |
| src/stdlib/helpers.rs | Adds expect_pattern helper for extracting compiled patterns from Value. |
| src/interpreter/mod.rs | Adapts interpreter to new native signatures and remaps errors to include line/column. |
| src/stdlib/pattern_test.rs | Updates assertions to match new standardized error phrasing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match value { | ||
| Value::Pattern(p) => Ok(Rc::clone(p)), | ||
| _ => Err(RuntimeError::new( | ||
| format!("Expected a pattern, got {}", value.type_name()), |
There was a problem hiding this comment.
expect_text uses the phrasing "Expected text, got ...", while expect_pattern uses "Expected a pattern, got ...". For consistency (and easier test/assert matching across helpers), consider standardizing the phrasing, e.g. "Expected pattern, got ..." (or update the other helpers to include the article).
Suggested change
| format!("Expected a pattern, got {}", value.type_name()), | |
| format!("Expected pattern, got {}", value.type_name()), |
Comment on lines
+438
to
+455
| /// Extracts a CompiledPattern value from a WFL Value, returning it as a reference-counted CompiledPattern. | ||
| /// | ||
| /// Returns an `Rc<CompiledPattern>` clone (incrementing the reference count) if the value | ||
| /// is a Pattern variant. | ||
| /// | ||
| /// # 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, with an error message | ||
| /// indicating the expected type and the actual type received. |
There was a problem hiding this comment.
The doc comment repeats the same “Returns an Rc<CompiledPattern> clone …” text in both the summary section and # Returns. Consider deduplicating to keep the docs concise (e.g., mention the Rc::clone behavior once, in # Returns).
Collaborator
Author
|
Incomplete Implementation: Critical: native_pattern_replace has a TODO and returns input text unchanged (line 129 in pattern.rs:129) Missing examples section in expect_pattern docstring compared to other helper functions |
Contributor
I have implemented the replacement logic in |
Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
Contributor
|
Claude finished @logbie's task —— View job Code Review for Pattern Refactoring PRCompleted comprehensive review of the pattern argument checking logic refactoring... Todo List:
🔍 Code Review SummaryThis is a well-executed refactoring that successfully deduplicates argument checking logic across pattern functions. The changes demonstrate good engineering practices and maintain backward compatibility. ✅ StrengthsCode Quality & Best Practices:
Type Safety & Error Handling:
Implementation Quality:
|
| Aspect | Impact | Notes |
|---|---|---|
| Code maintainability | ⬆️ Significantly improved | Reduced duplication, shared validation logic |
| Type safety | ➡️ Maintained | Strong typing preserved with helper functions |
| Performance | ➡️ No impact | Potentially slight improvement from reduced code size |
| Backward compatibility | ✅ Fully maintained | All existing functionality preserved |
| Test coverage | ➡️ Adequate | Good coverage with integration tests |
| Security | ✅ Maintained | Proper input validation and error handling |
✨ Conclusion
This refactoring successfully achieves its primary goals:
- ✅ Eliminates code duplication
- ✅ Standardizes argument validation
- ✅ Improves maintainability
- ✅ Preserves all functionality
- ✅ Maintains performance characteristics
The code is well-structured, follows Rust best practices, and demonstrates thoughtful engineering. The minor consistency issues noted are cosmetic and don't affect functionality.
Overall Assessment: ⭐⭐⭐⭐⭐ Excellent refactoring - Ready to merge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/stdlib/pattern.rs`:
- Around line 129-151: native_pattern_replace uses MatchResult.start/ end
(character indices) directly for &str slicing which can panic on multi-byte
UTF-8; mirror native_pattern_split by building the same char_to_byte mapping
from text_str.chars().enumerate().map(|(i, c)| (i, offset)) and convert match
indices via char_to_byte[m.start] and char_to_byte[m.end] (or compute byte
offsets for last_end too) before slicing; update variables in the loop and the
final slice to use byte offsets and return Arc::from(result) as before.
🧹 Nitpick comments (2)
src/stdlib/pattern.rs (2)
170-173: Nit: redundant variable rebinding.Line 171 creates an immutable
char_to_byte, then line 172 re-binds it as mutable solely to call.push()on line 173. Simplify by declaring itmutfrom the start.♻️ Suggested simplification
- let char_to_byte: Vec<usize> = text.char_indices().map(|(byte_idx, _)| byte_idx).collect(); - let mut char_to_byte = char_to_byte; - char_to_byte.push(text.len()); // Add final byte position + let mut char_to_byte: Vec<usize> = text.char_indices().map(|(byte_idx, _)| byte_idx).collect(); + char_to_byte.push(text.len()); // Add final byte position
41-119: Extract sharedMatchResult → Valueconversion to reduce duplication.
pattern_find_native(lines 50–74) andpattern_find_all_native(lines 91–115) contain nearly identical logic for converting a match result into aValue::Objectwithmatched_text,start,end, and optionalcaptures. Given this PR's goal is to deduplicate pattern logic, extracting a small helper (e.g.,fn match_result_to_value(m: MatchResult) -> Value) would further that objective.♻️ Sketch of a shared helper
fn match_result_to_value(match_result: crate::pattern::MatchResult) -> Value { let mut result_map = HashMap::new(); result_map.insert( "matched_text".to_string(), Value::Text(Arc::from(match_result.matched_text.as_str())), ); result_map.insert("start".to_string(), Value::Number(match_result.start as f64)); result_map.insert("end".to_string(), Value::Number(match_result.end as f64)); if !match_result.captures.is_empty() { let captures_map: HashMap<String, Value> = match_result .captures .into_iter() .map(|(name, value)| (name, Value::Text(Arc::from(value.as_str())))) .collect(); result_map.insert( "captures".to_string(), Value::Object(Rc::new(RefCell::new(captures_map))), ); } Value::Object(Rc::new(RefCell::new(result_map))) }Then
pattern_find_nativeandpattern_find_all_nativewould each just callmatch_result_to_value(m).
Collaborator
Author
|
In |
Contributor
Thank you for the detailed feedback. I have updated |
Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
Contributor
|
Claude Code is working… I'll analyze this and get back to you. |
Contributor
|
Claude finished @logbie's task —— View job Code Review for Pattern Refactoring PRCompleted comprehensive review of the pattern argument checking logic refactoring... Todo List:
🔍 Code Review SummaryThis is a well-executed refactoring that successfully deduplicates argument checking logic across pattern functions. The changes demonstrate good engineering practices and maintain backward compatibility. ✅ StrengthsCode Quality & Best Practices:
Type Safety & Error Handling:
Implementation Quality:
|
| Aspect | Impact | Notes |
|---|---|---|
| Code maintainability | ⬆️ Significantly improved | Reduced duplication, shared validation logic |
| Type safety | ➡️ Maintained | Strong typing preserved with helper functions |
| Performance | ➡️ No impact | Potentially slight improvement from reduced code size |
| Backward compatibility | ✅ Fully maintained | All existing functionality preserved |
| Test coverage | ➡️ Adequate | Good coverage with integration tests |
| Security | ✅ Maintained | Proper input validation and error handling |
✨ Conclusion
This refactoring successfully achieves its primary goals:
- ✅ Eliminates code duplication
- ✅ Standardizes argument validation
- ✅ Improves maintainability
- ✅ Preserves all functionality
- ✅ Maintains performance characteristics
The code is well-structured, follows Rust best practices, and demonstrates thoughtful engineering. The minor consistency issues noted are cosmetic and don't affect functionality.
Overall Assessment: ⭐⭐⭐⭐⭐ Excellent refactoring - Ready to merge
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 similar comment
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+467
to
+476
| pub fn expect_pattern(value: &Value) -> Result<Rc<crate::pattern::CompiledPattern>, RuntimeError> { | ||
| match value { | ||
| Value::Pattern(p) => Ok(Rc::clone(p)), | ||
| _ => Err(RuntimeError::new( | ||
| format!("Expected a pattern, got {}", value.type_name()), | ||
| 0, | ||
| 0, | ||
| )), | ||
| } | ||
| } |
There was a problem hiding this comment.
The new error message uses a different phrasing than other expect_* helpers (e.g., expect_text is "Expected text, got ..."). For consistency (and to simplify tests that match on message fragments), consider changing this to "Expected pattern, got {}" to align with the existing helper style.
Comment on lines
+139
to
+143
| let char_to_byte: Vec<usize> = text_str | ||
| .char_indices() | ||
| .map(|(byte_idx, _)| byte_idx) | ||
| .collect(); | ||
| let mut char_to_byte = char_to_byte; |
There was a problem hiding this comment.
This introduces an unnecessary shadowing assignment (let mut char_to_byte = char_to_byte;). Prefer declaring it mutable in the initial binding to reduce churn and make the intent clearer.
Suggested change
| let char_to_byte: Vec<usize> = text_str | |
| .char_indices() | |
| .map(|(byte_idx, _)| byte_idx) | |
| .collect(); | |
| let mut char_to_byte = char_to_byte; | |
| let mut char_to_byte: Vec<usize> = text_str | |
| .char_indices() | |
| .map(|(byte_idx, _)| byte_idx) | |
| .collect(); |
Comment on lines
+122
to
135
| pub fn native_pattern_replace(args: Vec<Value>) -> Result<Value, RuntimeError> { | ||
| check_arg_count("pattern_replace", &args, 3)?; | ||
|
|
||
| let text = expect_text(&args[0])?; | ||
| let pattern = expect_pattern(&args[1])?; | ||
| let replacement = expect_text(&args[2])?; | ||
|
|
||
| let text_str = text.as_ref(); | ||
| let matches = pattern.find_all(text_str); | ||
|
|
||
| // If no matches, return original text | ||
| if matches.is_empty() { | ||
| return Ok(Value::Text(text)); | ||
| } |
There was a problem hiding this comment.
This PR changes native_pattern_replace from the prior placeholder behavior (it previously returned the original text) to a real replacement implementation. I don’t see any new/updated tests in this diff that exercise replacement correctness (basic replacement, multiple matches, leading/trailing matches, and Unicode text where char/byte indices differ). Adding targeted tests would help prevent regressions in the new indexing/slicing logic.
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 similar comment
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
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.
Refactored
src/stdlib/pattern.rsto usesrc/stdlib/helpers.rsfor argument checking and type extraction.Added
expect_patterntosrc/stdlib/helpers.rs.Updated
native_pattern_replaceandnative_pattern_splitsignatures to standardized format.Updated
src/interpreter/mod.rsto handle new signatures and error mapping.Updated tests in
src/stdlib/pattern_test.rsto match new error messages.Verified with
cargo test,cargo clippy, andcargo fmt.PR created automatically by Jules for task 6239104481551978944 started by @logbie
Summary by CodeRabbit
Bug Fixes
New Features