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. |
📝 WalkthroughWalkthroughThis PR adds two new text helper functions ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stdlib/text.rs (1)
232-254:⚠️ Potential issue | 🔴 Critical
directionis moved and then reused inperform_pad.At line 238,
match directionconsumesdirection, then line 251 attempts to match it again. This is a compile-time ownership error.💡 Proposed fix
+#[derive(Copy, Clone)] enum PadDirection { Left, Right, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stdlib/text.rs` around lines 232 - 254, The PadDirection enum is being moved when matched in perform_pad (used to set func_name) and then reused later, causing an ownership error; make PadDirection Copy (and Clone) so it can be matched by value multiple times by adding #[derive(Copy, Clone)] to the PadDirection enum declaration (or alternatively change the first match to match &direction and use string slices), then recompile to ensure perform_pad (and its matches) no longer cause a move error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/stdlib/text.rs`:
- Around line 232-254: The PadDirection enum is being moved when matched in
perform_pad (used to set func_name) and then reused later, causing an ownership
error; make PadDirection Copy (and Clone) so it can be matched by value multiple
times by adding #[derive(Copy, Clone)] to the PadDirection enum declaration (or
alternatively change the first match to match &direction and use string slices),
then recompile to ensure perform_pad (and its matches) no longer cause a move
error.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the text stdlib module to reduce code duplication by introducing generic helper functions and consolidating similar implementations. The refactoring improves maintainability while preserving all existing functionality and test coverage.
Changes:
- Introduced
unary_text_opandbinary_text_predicategeneric helper functions insrc/stdlib/helpers.rsto eliminate boilerplate for argument checking, type extraction, and result wrapping - Added
perform_padhelper insrc/stdlib/text.rswithPadDirectionenum to consolidatepadleftandpadrightlogic - Refactored 7 text functions (
touppercase,tolowercase,trim,starts_with,ends_with,capitalize,reverse_text) to use the new helpers, reducing code duplication by approximately 60 lines
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/stdlib/helpers.rs | Added unary_text_op and binary_text_predicate helper functions with comprehensive documentation and unit tests |
| src/stdlib/text.rs | Refactored text manipulation functions to use new helpers; consolidated padleft/padright with perform_pad helper and PadDirection enum |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[test] | ||
| fn test_binary_text_predicate_arg_count() { | ||
| let op = |a: &str, b: &str| a.starts_with(b); | ||
| assert!(binary_text_predicate("test_pred", vec![], op).is_err()); | ||
| } |
There was a problem hiding this comment.
The test coverage for binary_text_predicate is incomplete. While there are tests for successful execution and incorrect argument count, there's no test verifying that the function correctly rejects arguments of the wrong type (e.g., passing a Number instead of Text). Consider adding a test similar to test_unary_text_op_wrong_type to ensure type validation works correctly.
| let text = expect_text(&args[0])?; | ||
| let trimmed = text.trim(); | ||
| Ok(Value::Text(Arc::from(trimmed))) | ||
| unary_text_op("trim", args, |s| Arc::from(s.trim())) |
There was a problem hiding this comment.
The explicit Arc::from() call in the trim closure is unnecessary. The helper function unary_text_op already converts the result to Arc<str> via .into() at line 216 of helpers.rs. For consistency with touppercase, tolowercase, and other text operations, and to avoid a redundant conversion, the closure should return &str or String directly. Change to: unary_text_op("trim", args, |s| s.trim())
|
Closing in triage: this PR is currently conflicted (DIRTY) and superseded by newer clean PRs in the same optimization/refactor sequence. Reopen or rebase if you want this exact change revived. |
Understood. Acknowledging that this work is superseded and stopping work on this PR. |
Summary of Changes
src/stdlib/text.rscontained significant boilerplate code for argument count checking, type extraction, and value wrapping across multiple native functions (touppercase,tolowercase,trim,starts_with,ends_with,padleft,padright,capitalize,reverse_text).unary_text_opandbinary_text_predicategeneric helper functions insrc/stdlib/helpers.rsto abstract common text operations.perform_padhelper insrc/stdlib/text.rsto consolidatepadleftandpadrightlogic.src/stdlib/text.rsto use these helpers.src/stdlib/helpers.rs.Verification Checklist
cargo fmtexecuted and passed.cargo clippyreturned no warnings or errors.cargo testsuites passed (398 unit tests passed; 6 integration tests failed due to missing release binary, unrelated to changes).PR created automatically by Jules for task 13566135231111813240 started by @logbie
Summary by CodeRabbit
Refactor
Tests