[JULES] Scheduled Maintenance: Refactor redundant expect_* boilerplate in stdlib helpers - #427
Conversation
…acro Consolidates the repetitive `match` boilerplate found in the 7 `expect_*` functions (e.g. `expect_number`, `expect_text`, etc.) into a single declarative macro. This ensures consistent error messaging, simplifies the module, and drastically reduces code duplication without altering behavior. 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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughA macro named Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![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
Refactors the stdlib expect_* type-extraction helpers by introducing a declarative macro to remove repeated boilerplate while keeping the same runtime error shape/messages across extractors.
Changes:
- Added a
generate_expect!macro to generateexpect_*extractors with uniform error formatting. - Replaced the handwritten
expect_number/text/list/bool/date/time/datetimeimplementations with macro invocations. - Kept per-extractor rustdoc by passing
///docs through the macro.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// | ||
| /// Returns an `Rc<str>` clone (incrementing the reference count) if the value is a Text variant. | ||
| /// The underlying string data is not copied, only the reference count is incremented. | ||
| /// |
There was a problem hiding this comment.
These docs say the function returns an Rc<str> clone, but the return type is Arc<str> and the implementation uses Arc::clone. Please align the rustdoc with the actual Arc<str> API.
| $(#[$meta])* | ||
| pub fn $func_name(value: &Value) -> Result<$return_type, RuntimeError> { | ||
| match value { | ||
| Value::$variant(v) => Ok($extract(v)), |
There was a problem hiding this comment.
Ok($extract(v)) will not work when $extract is provided as a closure literal (as in the current invocations). The expansion becomes Ok(|x| ... (v)), which parses as a closure whose body calls v, and fails to compile. Wrap the extractor in parentheses (e.g., call it as ($extract)(v)) or require an identifier/path instead of a free-form expression.
| Value::$variant(v) => Ok($extract(v)), | |
| Value::$variant(v) => Ok(($extract)(v)), |
| /// let num = expect_number(&args[0])?; | ||
| /// Ok(Value::Number(num.abs())) | ||
| /// } | ||
| /// ``` | ||
| pub fn expect_number(value: &Value) -> Result<f64, RuntimeError> { | ||
| match value { | ||
| Value::Number(n) => Ok(*n), | ||
| _ => Err(RuntimeError::new( | ||
| format!("Expected a number, got {}", value.type_name()), | ||
| 0, | ||
| 0, | ||
| )), | ||
| } | ||
| macro_rules! generate_expect { |
There was a problem hiding this comment.
The expect_number rustdoc block directly above generate_expect! now applies to the generate_expect macro (and duplicates the rustdoc you pass into generate_expect!(..., expect_number, ...)). Consider removing the earlier rustdoc block or moving the macro definition above it so the docs attach to the intended item.
| /// Returns an `Rc<str>` to enable efficient memory sharing without copying the string | ||
| /// data. This is the standard way to extract text values in the WFL runtime. | ||
| /// |
There was a problem hiding this comment.
These docs say Rc<str>, but Value::Text is Arc<str> and expect_text returns Arc<str>. Update the rustdoc to reference Arc<str> (and describe atomic ref-counting) to avoid misleading API consumers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stdlib/helpers.rs`:
- Around line 381-405: The docs for expect_text incorrectly mention Rc<str> and
show Rc::from(...) even though the implementation uses Arc<str>/Arc::clone;
update the doc comments and the example to refer to Arc<str>, use Arc::from(...)
in the example, and mention Arc::clone where relevant so the documentation
matches the expect_text implementation and its return type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| /// Returns an `Rc<str>` to enable efficient memory sharing without copying the string | ||
| /// data. This is the standard way to extract text values in the WFL runtime. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `value` - The WFL Value to extract from | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// Returns an `Rc<str>` clone (incrementing the reference count) if the value is a Text variant. | ||
| /// The underlying string data is not copied, only the reference count is incremented. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns `RuntimeError` if the value is not a Text, with an error message | ||
| /// indicating the expected type and the actual type received. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ```ignore | ||
| /// pub fn native_uppercase(args: Vec<Value>) -> Result<Value, RuntimeError> { | ||
| /// check_arg_count("uppercase", &args, 1)?; | ||
| /// let text = expect_text(&args[0])?; | ||
| /// Ok(Value::Text(Rc::from(text.to_uppercase()))) | ||
| /// } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining Rc/Arc doc inconsistencies in this helper file.
rg -n 'Rc<str>|Rc::from\(text\.to_uppercase\(\)\)|Arc<str>|Arc::from' src/stdlib/helpers.rsRepository: WebFirstLanguage/wfl
Length of output: 686
Fix Arc/Rc mismatch in expect_text documentation.
The generated function returns Arc<str>, but the doc block still incorrectly describes Rc<str> and uses Rc::from(...) in the example. Lines 381 and 390 reference Rc<str>, and line 404 shows Rc::from(text.to_uppercase()), but the actual implementation (lines 409–411) uses Arc<str> and Arc::clone.
📝 Proposed doc fix
- /// Returns an `Rc<str>` to enable efficient memory sharing without copying the string
+ /// Returns an `Arc<str>` to enable efficient memory sharing without copying the string
@@
- /// Returns an `Rc<str>` clone (incrementing the reference count) if the value is a Text variant.
+ /// Returns an `Arc<str>` clone (incrementing the reference count) if the value is a Text variant.
@@
- /// Ok(Value::Text(Rc::from(text.to_uppercase())))
+ /// Ok(Value::Text(Arc::from(text.to_uppercase())))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Returns an `Rc<str>` to enable efficient memory sharing without copying the string | |
| /// data. This is the standard way to extract text values in the WFL runtime. | |
| /// | |
| /// # Arguments | |
| /// | |
| /// * `value` - The WFL Value to extract from | |
| /// | |
| /// # Returns | |
| /// | |
| /// Returns an `Rc<str>` clone (incrementing the reference count) if the value is a Text variant. | |
| /// The underlying string data is not copied, only the reference count is incremented. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Returns `RuntimeError` if the value is not a Text, with an error message | |
| /// indicating the expected type and the actual type received. | |
| /// | |
| /// # Examples | |
| /// | |
| /// ```ignore | |
| /// pub fn native_uppercase(args: Vec<Value>) -> Result<Value, RuntimeError> { | |
| /// check_arg_count("uppercase", &args, 1)?; | |
| /// let text = expect_text(&args[0])?; | |
| /// Ok(Value::Text(Rc::from(text.to_uppercase()))) | |
| /// } | |
| /// Returns an `Arc<str>` to enable efficient memory sharing without copying the string | |
| /// data. This is the standard way to extract text values in the WFL runtime. | |
| /// | |
| /// # Arguments | |
| /// | |
| /// * `value` - The WFL Value to extract from | |
| /// | |
| /// # Returns | |
| /// | |
| /// Returns an `Arc<str>` clone (incrementing the reference count) if the value is a Text variant. | |
| /// The underlying string data is not copied, only the reference count is incremented. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Returns `RuntimeError` if the value is not a Text, with an error message | |
| /// indicating the expected type and the actual type received. | |
| /// | |
| /// # Examples | |
| /// | |
| /// |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stdlib/helpers.rs` around lines 381 - 405, The docs for expect_text
incorrectly mention Rc<str> and show Rc::from(...) even though the
implementation uses Arc<str>/Arc::clone; update the doc comments and the example
to refer to Arc<str>, use Arc::from(...) in the example, and mention Arc::clone
where relevant so the documentation matches the expect_text implementation and
its return type.
Summary of Changes
src/stdlib/helpers.rscontained 7 heavily repeatedexpect_*functions (expect_number,expect_text,expect_list,expect_bool,expect_date,expect_time,expect_datetime). Each function followed identical logic: attempt to extract a specific type from aValueenum and return aRuntimeErroron failure, resulting in unnecessary boilerplate.generate_expect!to stamp out these implementations cleanly.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 7776030076748350677 started by @logbie
Summary by CodeRabbit