-
Notifications
You must be signed in to change notification settings - Fork 0
[Refactor] Deduplicate pattern argument checking logic #334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7195f79
9bcf510
f8ed226
1a48ac9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -434,3 +434,43 @@ pub fn expect_datetime(value: &Value) -> Result<Rc<chrono::NaiveDateTime>, Runti | |||||
| )), | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// 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. | ||||||
| /// | ||||||
| /// # Examples | ||||||
| /// | ||||||
| /// ```ignore | ||||||
| /// pub fn pattern_matches_native(args: Vec<Value>) -> Result<Value, RuntimeError> { | ||||||
| /// check_arg_count("pattern_matches", &args, 2)?; | ||||||
| /// let compiled_pattern = expect_pattern(&args[1])?; | ||||||
| /// // Use compiled_pattern.matches(...) | ||||||
| /// Ok(Value::Bool(true)) | ||||||
| /// } | ||||||
| /// ``` | ||||||
| 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()), | ||||||
|
||||||
| format!("Expected a pattern, got {}", value.type_name()), | |
| format!("Expected pattern, got {}", value.type_name()), |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||||||||||||||||
| use super::helpers::{check_arg_count, expect_pattern, expect_text}; | ||||||||||||||||||||
| use crate::interpreter::environment::Environment; | ||||||||||||||||||||
| use crate::interpreter::error::RuntimeError; | ||||||||||||||||||||
| use crate::interpreter::value::Value; | ||||||||||||||||||||
|
|
@@ -28,74 +29,24 @@ pub fn register(env: &mut Environment) { | |||||||||||||||||||
| /// 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, | ||||||||||||||||||||
| )); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| check_arg_count("pattern_matches", &args, 2)?; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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, | ||||||||||||||||||||
| )); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| let text_str = expect_text(&args[0])?; | ||||||||||||||||||||
| let compiled_pattern = expect_pattern(&args[1])?; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let compiled_pattern = match &args[1] { | ||||||||||||||||||||
| Value::Pattern(p) => p, | ||||||||||||||||||||
| _ => { | ||||||||||||||||||||
| return Err(RuntimeError::new( | ||||||||||||||||||||
| "Second argument to pattern_matches must be a compiled pattern".to_string(), | ||||||||||||||||||||
| 0, | ||||||||||||||||||||
| 0, | ||||||||||||||||||||
| )); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let matches = compiled_pattern.matches(text_str); | ||||||||||||||||||||
| let matches = compiled_pattern.matches(text_str.as_ref()); | ||||||||||||||||||||
| Ok(Value::Bool(matches)) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// 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)?; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let text_str = match &args[0] { | ||||||||||||||||||||
| Value::Text(s) => s.as_ref(), | ||||||||||||||||||||
| _ => { | ||||||||||||||||||||
| return Err(RuntimeError::new( | ||||||||||||||||||||
| "First argument to pattern_find must be text".to_string(), | ||||||||||||||||||||
| 0, | ||||||||||||||||||||
| 0, | ||||||||||||||||||||
| )); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let compiled_pattern = match &args[1] { | ||||||||||||||||||||
| Value::Pattern(p) => p, | ||||||||||||||||||||
| _ => { | ||||||||||||||||||||
| return Err(RuntimeError::new( | ||||||||||||||||||||
| "Second argument to pattern_find must be a compiled pattern".to_string(), | ||||||||||||||||||||
| 0, | ||||||||||||||||||||
| 0, | ||||||||||||||||||||
| )); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| let text_str = expect_text(&args[0])?; | ||||||||||||||||||||
| let compiled_pattern = expect_pattern(&args[1])?; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| match compiled_pattern.find(text_str) { | ||||||||||||||||||||
| match compiled_pattern.find(text_str.as_ref()) { | ||||||||||||||||||||
| Some(match_result) => { | ||||||||||||||||||||
| let mut result_map = HashMap::new(); | ||||||||||||||||||||
| result_map.insert( | ||||||||||||||||||||
|
|
@@ -129,37 +80,12 @@ pub fn pattern_find_native(args: Vec<Value>) -> Result<Value, RuntimeError> { | |||||||||||||||||||
| /// 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, | ||||||||||||||||||||
| )); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| check_arg_count("pattern_find_all", &args, 2)?; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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, | ||||||||||||||||||||
| )); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let compiled_pattern = match &args[1] { | ||||||||||||||||||||
| Value::Pattern(p) => p, | ||||||||||||||||||||
| _ => { | ||||||||||||||||||||
| return Err(RuntimeError::new( | ||||||||||||||||||||
| "Second argument to pattern_find_all must be a compiled pattern".to_string(), | ||||||||||||||||||||
| 0, | ||||||||||||||||||||
| 0, | ||||||||||||||||||||
| )); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| let text_str = expect_text(&args[0])?; | ||||||||||||||||||||
| let compiled_pattern = expect_pattern(&args[1])?; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let matches = compiled_pattern.find_all(text_str); | ||||||||||||||||||||
| let matches = compiled_pattern.find_all(text_str.as_ref()); | ||||||||||||||||||||
| let mut result_list = Vec::new(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| for match_result in matches { | ||||||||||||||||||||
|
|
@@ -193,98 +119,85 @@ pub fn pattern_find_all_native(args: Vec<Value>) -> Result<Value, RuntimeError> | |||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// Native function for pattern replacement (called by interpreter) | ||||||||||||||||||||
| pub fn native_pattern_replace( | ||||||||||||||||||||
| args: Vec<Value>, | ||||||||||||||||||||
| line: usize, | ||||||||||||||||||||
| column: usize, | ||||||||||||||||||||
| ) -> Result<Value, RuntimeError> { | ||||||||||||||||||||
| if args.len() != 3 { | ||||||||||||||||||||
| return Err(RuntimeError::new( | ||||||||||||||||||||
| "pattern_replace requires exactly 3 arguments".to_string(), | ||||||||||||||||||||
| line, | ||||||||||||||||||||
| column, | ||||||||||||||||||||
| )); | ||||||||||||||||||||
| 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)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+122
to
135
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| let text = match &args[0] { | ||||||||||||||||||||
| Value::Text(t) => t.as_ref(), | ||||||||||||||||||||
| _ => { | ||||||||||||||||||||
| return Err(RuntimeError::new( | ||||||||||||||||||||
| "First argument must be text".to_string(), | ||||||||||||||||||||
| line, | ||||||||||||||||||||
| column, | ||||||||||||||||||||
| )); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| // Build character-to-byte index mapping | ||||||||||||||||||||
| // This is needed because match indices are character offsets, but string slicing uses byte offsets | ||||||||||||||||||||
| let char_to_byte: Vec<usize> = text_str | ||||||||||||||||||||
| .char_indices() | ||||||||||||||||||||
| .map(|(byte_idx, _)| byte_idx) | ||||||||||||||||||||
| .collect(); | ||||||||||||||||||||
| let mut char_to_byte = char_to_byte; | ||||||||||||||||||||
|
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; | |
| let mut char_to_byte: Vec<usize> = text_str | |
| .char_indices() | |
| .map(|(byte_idx, _)| byte_idx) | |
| .collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theRc::clonebehavior once, in# Returns).