[ISSUE #92]✨Enhance CheetahString with support for character and string patterns in starts_with, ends_with, contains, and split methods #93
Conversation
…ing patterns in starts_with, ends_with, contains, and split methods
📝 WalkthroughWalkthroughEnhances CheetahString's pattern matching API to accept both character and string patterns. Introduces new traits (StrPattern, SplitPattern) and supporting types to enable polymorphic pattern matching. Updates method signatures for starts_with, ends_with, contains, and split. Includes comprehensive examples and test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @examples/test_empty_pattern.rs:
- Around line 19-25: The example shows a behavioral mismatch: CheetahString's
split("") returns the whole string while Rust's standard String.split("") yields
splits between every character; update the implementation or example to resolve
this. Either (A) change CheetahString::split (and any related split internals)
to mirror std::string::String::split on an empty pattern so split("") produces
empty-string-separated characters, (B) if intentional, add a clear API doc
comment on CheetahString::split and surface this limitation in examples and
tests (update examples/test_empty_pattern.rs and tests/split_edge_cases.rs to
document the difference), or (C) make CheetahString::split panic on empty
pattern to avoid silent divergence; pick one approach and apply it consistently
across the split implementation, its public docs, the example
(examples/test_empty_pattern.rs) and the test test_empty_pattern in
tests/split_edge_cases.rs.
In @src/cheetah_string.rs:
- Around line 1410-1420: The current DoubleEndedIterator implementation for
SplitWrapper panics in SplitWrapper::Str branch; change it to return None
instead of panic to avoid unexpected runtime aborts: in impl<'a>
DoubleEndedIterator for SplitWrapper<'a> ensure fn next_back(&mut self) matches
SplitWrapper::Str => return None (keeping SplitWrapper::Char delegating to
iter.next_back()), and update the doc comment on the split function (or
SplitWrapper) to note that reverse iteration over string-pattern splits yields
None rather than supporting reverse traversal.
In @tests/split_edge_cases.rs:
- Around line 63-67: The test file incorrectly defines a main() function that
calls test_split_edge_cases() and test_empty_pattern(); remove the entire main()
function so tests are run by Rust's #[test] harness instead ( locate and delete
the main() definition that invokes test_split_edge_cases and test_empty_pattern
); rely on the existing #[test] functions and run them via cargo test.
- Around line 54-61: The test_empty_pattern currently only prints the split
result; update the test (function test_empty_pattern) to assert that splitting
with an empty pattern yields the standard library behavior: collect the iterator
into a Vec<&str> and assert_eq! it to ["", "h", "e", "l", "l", "o", ""] (or
assert length == 7 and individual elements) for
CheetahString::from("hello").split(""); ensure you reference the existing parts
variable and replace the println! with the appropriate assertions.
🧹 Nitpick comments (4)
src/cheetah_string.rs (2)
1326-1340:SplitStris missingCloneimplementation, limiting usability.
EmptyPatternStatederivesClone, butSplitStrdoes not implementClone. This means users cannot clone aSplitWrapper::Striterator, which differs from the standard library'sstr::Splitbehavior.Consider implementing Clone for SplitStr
+impl<'a> Clone for SplitStr<'a> { + fn clone(&self) -> Self { + SplitStr { + string: self.string, + pattern: self.pattern, + finished: self.finished, + empty_pattern_state: self.empty_pattern_state.clone(), + } + } +}
1299-1303: Lifetime mismatch inSplitPatternimplementation forchar.The impl declares
SplitPattern<'_>(anonymous lifetime) but the trait is generic over'a. This works becausechardoesn't borrow anything, but it's slightly inconsistent with the&'a strimplementation.Consider using explicit lifetime for consistency
-impl SplitPattern<'_> for char { - fn split_str(self, s: &str) -> SplitWrapper<'_> { +impl<'a> SplitPattern<'a> for char { + fn split_str(self, s: &'a str) -> SplitWrapper<'a> { SplitWrapper::Char(s.split(self)) } }tests/patterns.rs (1)
34-39: Consider adding a test for split with string pattern to match test_split_char.While
test_split_strtests basic string splitting, adding a#[should_panic]test for reverse iteration on string patterns would document the expected behavior.Suggested test addition
#[test] #[should_panic(expected = "split with string pattern does not support reverse iteration")] fn test_split_str_reverse_panics() { let s = CheetahString::from("a::b::c"); let _: Vec<&str> = s.split("::").rev().collect(); }tests/sso.rs (1)
73-83: The SSO Unicode boundary test no longer tests near the 23-byte boundary.The original test likely used Chinese characters (3 bytes each) to get close to the 23-byte SSO limit. The new test uses 14 bytes (7 × 2-byte chars), which is well under the limit. Consider adding a test that approaches the boundary with multi-byte characters.
Consider adding a boundary test with multi-byte Unicode
#[test] fn test_sso_unicode_at_boundary() { // 11 2-byte chars = 22 bytes (just under 23-byte SSO limit) let s = CheetahString::from("\u{00E9}\u{00E7}\u{00F1}\u{00FC}\u{00E0}\u{00F6}\u{00E4}\u{00EB}\u{00EF}\u{00F2}\u{00F9}"); assert_eq!(s.len(), 22); // 12 2-byte chars = 24 bytes (just over SSO limit, uses Arc) let s2 = CheetahString::from("\u{00E9}\u{00E7}\u{00F1}\u{00FC}\u{00E0}\u{00F6}\u{00E4}\u{00EB}\u{00EF}\u{00F2}\u{00F9}\u{00FA}"); assert_eq!(s2.len(), 24); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
examples/comprehensive_patterns.rsexamples/pattern_matching.rsexamples/test_empty_pattern.rsexamples/test_split_behavior.rssrc/cheetah_string.rssrc/lib.rstests/api_extensions.rstests/basic.rstests/comprehensive_tests.rstests/patterns.rstests/split_edge_cases.rstests/sso.rs
🧰 Additional context used
🧬 Code graph analysis (7)
tests/sso.rs (1)
src/cheetah_string.rs (14)
from(30-32)from(37-39)from(44-46)from(58-62)from(82-86)from(91-96)from(101-106)from(120-122)from(163-165)from(170-172)from(177-208)from(243-245)len(428-438)as_str(390-412)
examples/pattern_matching.rs (1)
src/cheetah_string.rs (2)
starts_with(468-473)ends_with(504-509)
tests/split_edge_cases.rs (2)
examples/test_empty_pattern.rs (1)
main(3-37)examples/test_split_behavior.rs (1)
main(3-72)
examples/test_empty_pattern.rs (4)
src/serde.rs (1)
cheetah_string(27-89)examples/test_split_behavior.rs (1)
main(3-72)tests/split_edge_cases.rs (1)
main(63-67)src/cheetah_string.rs (13)
len(428-438)from(30-32)from(37-39)from(44-46)from(58-62)from(82-86)from(91-96)from(101-106)from(120-122)from(163-165)from(170-172)from(177-208)from(243-245)
examples/comprehensive_patterns.rs (4)
examples/pattern_matching.rs (1)
main(3-27)examples/test_empty_pattern.rs (1)
main(3-37)examples/test_split_behavior.rs (1)
main(3-72)src/cheetah_string.rs (15)
from(30-32)from(37-39)from(44-46)from(58-62)from(82-86)from(91-96)from(101-106)from(120-122)from(163-165)from(170-172)from(177-208)from(243-245)starts_with(468-473)ends_with(504-509)contains(540-545)
tests/basic.rs (1)
src/cheetah_string.rs (13)
from(30-32)from(37-39)from(44-46)from(58-62)from(82-86)from(91-96)from(101-106)from(120-122)from(163-165)from(170-172)from(177-208)from(243-245)len(428-438)
tests/api_extensions.rs (1)
src/cheetah_string.rs (4)
contains(540-545)starts_with(468-473)ends_with(504-509)find(575-577)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (28)
src/cheetah_string.rs (7)
468-472: LGTM!The
starts_withimplementation correctly dispatches to the underlyingstr::starts_withfor both char and string patterns. The use of the sealed trait pattern ensures extensibility is controlled.
504-508: LGTM!The
ends_withimplementation mirrorsstarts_withcorrectly, delegating to the appropriatestr::ends_withvariant based on pattern type.
540-544: LGTM!The
containsimplementation correctly handles both char and string patterns via delegation.
690-691: Good improvement: returning concretestd::str::Chars<'_>enablesDoubleEndedIterator.This change allows users to call
.rev()on the chars iterator, which wasn't possible with the previousimpl Iteratorreturn type.
1250-1261: LGTM: Sealed trait pattern correctly restricts trait implementations.The sealed trait pattern prevents external crates from implementing
StrPatternandSplitPattern, which is appropriate for maintaining API stability.
1350-1388: Empty pattern split behavior is correct.The implementation correctly matches Rust's
str::split("")behavior: it yields an empty string first, then each UTF-8 character via CharIndices, then an empty string last. Example:"abc".split("") == ["", "a", "b", "c", ""]. The use of CharIndices properly handles multi-byte UTF-8 characters.
653-658:splitreturn type changed fromimpl IteratortoSplitWrapper<'a>— a breaking change that should be documented.The method signature was changed from
pub fn split<'a>(&'a self, pat: &'a str) -> impl Iterator<Item = &'a str>topub fn split<'a, P>(&'a self, pat: P) -> SplitWrapper<'a>whereP: SplitPattern<'a>. WhileSplitWrapper<'a>implementsIterator, code with explicit type annotations (e.g., storing the result in a typed variable or struct field) will break and require updates. Most common usage patterns (for loops,.collect(), method chaining) are unaffected since the iterator contract is preserved. This change is intentional and adds useful functionality (supporting both char and string patterns), but it warrants release notes or documentation for users upgrading from 0.1.x.tests/patterns.rs (1)
1-81: LGTM: Comprehensive test coverage for new pattern API.The tests cover the key functionality: char patterns with
starts_with,ends_with,contains, andsplit. The double-ended iterator tests forchars()andsplit(char)are valuable.tests/basic.rs (2)
48-50: LGTM: Unicode test updated to use Latin extended characters.The test correctly validates that a 2-byte UTF-8 character (é, \u{00E9}) is handled properly.
236-238: LGTM: Unicode string test updated consistently.Four 2-byte Latin extended characters (8 bytes total) correctly replace the previous 4-character test.
examples/pattern_matching.rs (1)
1-27: LGTM: Clean example demonstrating the new pattern API.This example effectively demonstrates
starts_withandends_withwith both char and&strpatterns, which directly addresses the issue #92 requirements.tests/api_extensions.rs (4)
117-119: LGTM: Unicode chars test updated to use escape sequences.Consistent with the changes in other test files.
282-286: LGTM: Unicode query tests correctly validate pattern matching with extended Latin characters.The test verifies
contains,starts_with,ends_with, andfindwork correctly with Unicode.
291-296: LGTM: Unicode transformation test validates case conversion.Correctly tests that é (\u{00E9}) uppercases to É (\u{00C9}).
301-303: LGTM: Unicode split test validates splitting with Unicode content.Tests that the split functionality works correctly with Unicode characters.
tests/sso.rs (1)
66-71: LGTM: Unicode short string test updated.Test validates that short Unicode strings (2 chars × 2 bytes = 4 bytes) work correctly with SSO.
src/lib.rs (1)
27-27: SplitWrapper must remain public sincesplit()returns it directly.However, SplitPattern and StrPattern should be marked
#[doc(hidden)]if the crate doesn't intend for users to write generic code with explicit trait bounds. Currently, these traits are public and re-exported, but their methods are already marked#[doc(hidden)], suggesting they're not intended for direct use. Since the test suite and typical usage involve passing concrete types (char,&str) directly to these methods rather than writing generic bounds, hiding the traits would clarify that they're implementation details.SplitStr should be marked
#[doc(hidden)]as it's only instantiated internally as a variant of SplitWrapper, which already hides its variants.examples/comprehensive_patterns.rs (1)
1-48: LGTM! Comprehensive demonstration of pattern-based API.This example effectively demonstrates all the key features of the enhanced pattern-based API including
starts_with,ends_with,contains, andsplitwith both character and string patterns. The output messages are clear and the examples cover practical use cases.examples/test_split_behavior.rs (1)
1-72: LGTM! Effective validation example.This example provides valuable validation by comparing CheetahString split behavior with the standard library across comprehensive edge cases including empty strings, leading/trailing separators, and consecutive separators. The clear [OK]/[FAIL] indicators make it easy to verify correctness.
tests/split_edge_cases.rs (1)
3-52: LGTM! Comprehensive edge case coverage.The test cases thoroughly cover important edge cases for split functionality including empty strings, leading/trailing separators, consecutive separators, and multi-character string patterns. All assertions are clear and correct.
tests/comprehensive_tests.rs (8)
3-13: LGTM! Unicode split tests are clear and correct.The tests properly validate splitting with both character and string patterns, with clear assertions.
15-26: LGTM! Iterator exhaustion behavior is properly tested.Good practice to verify that multiple calls to
next()after exhaustion continue returningNone.
28-41: LGTM! Double-ended iterator behavior is well-tested.The tests validate both
rev()and interleavednext()/next_back()calls, which are important forDoubleEndedIteratorcorrectness.
43-48: LGTM! Documented panic behavior for string pattern reverse.The test correctly documents that string pattern splits do not support reverse iteration with a clear panic message.
50-65: LGTM! Pattern trait tests cover both char and string patterns.Comprehensive validation of
starts_with,ends_with,contains, andsplitwith both pattern types.
67-79: LGTM! Character-level double-ended iteration is properly validated.Tests both forward and backward traversal of characters.
81-90: LGTM! Long string handling beyond inline capacity is tested.This validates that split works correctly for strings that exceed the small string optimization threshold.
92-106: LGTM! Special character patterns are well-covered.Tests tab, newline, and backslash separators, which are common special cases.
| if std_result == cheetah_result { | ||
| println!("\n[OK] Behavior matches"); | ||
| } else { | ||
| println!("\n[FAIL] Behavior mismatch!"); | ||
| println!("Note: Standard library splits empty pattern between each character"); | ||
| println!("Current implementation returns the whole string for simplicity"); | ||
| } |
There was a problem hiding this comment.
Address the empty pattern behavior mismatch with standard library.
This example documents a significant behavioral inconsistency: the standard library's split("") splits between each character, but CheetahString returns the whole string "for simplicity". This breaks compatibility with Rust's standard library semantics and will confuse users who expect standard behavior.
The example itself prints "[FAIL] Behavior mismatch!" which indicates this is a known issue. Users migrating from String to CheetahString will encounter unexpected behavior.
Consider either:
- Implementing standard library-compatible empty pattern behavior
- Documenting this limitation prominently in the API documentation if there's a technical reason for the difference
- Panicking on empty patterns (like some other string operations do) to prevent silent behavioral differences
Based on the related code in tests/split_edge_cases.rs which has a test called test_empty_pattern that only prints without assertions, it appears this behavioral difference may still be under development.
🤖 Prompt for AI Agents
In @examples/test_empty_pattern.rs around lines 19-25, The example shows a
behavioral mismatch: CheetahString's split("") returns the whole string while
Rust's standard String.split("") yields splits between every character; update
the implementation or example to resolve this. Either (A) change
CheetahString::split (and any related split internals) to mirror
std::string::String::split on an empty pattern so split("") produces
empty-string-separated characters, (B) if intentional, add a clear API doc
comment on CheetahString::split and surface this limitation in examples and
tests (update examples/test_empty_pattern.rs and tests/split_edge_cases.rs to
document the difference), or (C) make CheetahString::split panic on empty
pattern to avoid silent divergence; pick one approach and apply it consistently
across the split implementation, its public docs, the example
(examples/test_empty_pattern.rs) and the test test_empty_pattern in
tests/split_edge_cases.rs.
| impl<'a> DoubleEndedIterator for SplitWrapper<'a> { | ||
| fn next_back(&mut self) -> Option<Self::Item> { | ||
| match self { | ||
| SplitWrapper::Char(iter) => iter.next_back(), | ||
| SplitWrapper::Str(_) => { | ||
| // String pattern split doesn't support reverse iteration | ||
| // This is consistent with std::str::Split<&str> | ||
| panic!("split with string pattern does not support reverse iteration") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Panic in DoubleEndedIterator::next_back is a significant UX concern.
While the comment notes this is "consistent with std::str::Split<&str>", panicking at runtime when users call .rev() on a string-pattern split is unexpected. Consider returning None or documenting this prominently.
Suggested alternatives
Option 1: Document the panic in the split method's doc comment:
/// Splits the string by the given pattern.
///
+/// # Panics
+///
+/// When using a string pattern (not a char), calling `.rev()` or `.next_back()`
+/// on the returned iterator will panic. Use a char pattern if reverse iteration is needed.
+///
/// # ExamplesOption 2: Return None instead of panicking (changes behavior but is safer):
fn next_back(&mut self) -> Option<Self::Item> {
match self {
SplitWrapper::Char(iter) => iter.next_back(),
- SplitWrapper::Str(_) => {
- // String pattern split doesn't support reverse iteration
- // This is consistent with std::str::Split<&str>
- panic!("split with string pattern does not support reverse iteration")
- }
+ SplitWrapper::Str(_) => None,
}
}📝 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.
| impl<'a> DoubleEndedIterator for SplitWrapper<'a> { | |
| fn next_back(&mut self) -> Option<Self::Item> { | |
| match self { | |
| SplitWrapper::Char(iter) => iter.next_back(), | |
| SplitWrapper::Str(_) => { | |
| // String pattern split doesn't support reverse iteration | |
| // This is consistent with std::str::Split<&str> | |
| panic!("split with string pattern does not support reverse iteration") | |
| } | |
| } | |
| } | |
| impl<'a> DoubleEndedIterator for SplitWrapper<'a> { | |
| fn next_back(&mut self) -> Option<Self::Item> { | |
| match self { | |
| SplitWrapper::Char(iter) => iter.next_back(), | |
| SplitWrapper::Str(_) => None, | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/cheetah_string.rs around lines 1410-1420, The current
DoubleEndedIterator implementation for SplitWrapper panics in SplitWrapper::Str
branch; change it to return None instead of panic to avoid unexpected runtime
aborts: in impl<'a> DoubleEndedIterator for SplitWrapper<'a> ensure fn
next_back(&mut self) matches SplitWrapper::Str => return None (keeping
SplitWrapper::Char delegating to iter.next_back()), and update the doc comment
on the split function (or SplitWrapper) to note that reverse iteration over
string-pattern splits yields None rather than supporting reverse traversal.
| #[test] | ||
| fn test_empty_pattern() { | ||
| // Empty pattern should split between each character | ||
| let s = CheetahString::from("hello"); | ||
| let parts: Vec<&str> = s.split("").collect(); | ||
| println!("Empty pattern: {:?}", parts); | ||
| // Standard library behavior: empty pattern splits between each character | ||
| } |
There was a problem hiding this comment.
Add assertions to validate empty pattern behavior.
The test prints diagnostic output but lacks assertions to verify the expected behavior. Without assertions, this test cannot fail and provides no validation.
🔎 Suggested fix
Based on the comment and the related example in examples/test_empty_pattern.rs, add assertions that match the standard library behavior:
#[test]
fn test_empty_pattern() {
// Empty pattern should split between each character
let s = CheetahString::from("hello");
let parts: Vec<&str> = s.split("").collect();
println!("Empty pattern: {:?}", parts);
- // Standard library behavior: empty pattern splits between each character
+
+ // Standard library behavior for comparison
+ let std_parts: Vec<&str> = "hello".split("").collect();
+ assert_eq!(parts, std_parts, "Empty pattern should match standard library behavior");
}🤖 Prompt for AI Agents
In @tests/split_edge_cases.rs around lines 54-61, The test_empty_pattern
currently only prints the split result; update the test (function
test_empty_pattern) to assert that splitting with an empty pattern yields the
standard library behavior: collect the iterator into a Vec<&str> and assert_eq!
it to ["", "h", "e", "l", "l", "o", ""] (or assert length == 7 and individual
elements) for CheetahString::from("hello").split(""); ensure you reference the
existing parts variable and replace the println! with the appropriate
assertions.
| fn main() { | ||
| test_split_edge_cases(); | ||
| test_empty_pattern(); | ||
| println!("\nAll tests passed!"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the main() function from test file.
Test files should not contain a main() function. When running cargo test, the test framework executes functions marked with #[test], not main(). This creates confusion about test execution and is non-idiomatic.
🔎 Suggested fix
Remove the main() function entirely:
-fn main() {
- test_split_edge_cases();
- test_empty_pattern();
- println!("\nAll tests passed!");
-}If you need to run these tests manually for debugging, use cargo test instead.
📝 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.
| fn main() { | |
| test_split_edge_cases(); | |
| test_empty_pattern(); | |
| println!("\nAll tests passed!"); | |
| } |
🤖 Prompt for AI Agents
In @tests/split_edge_cases.rs around lines 63-67, The test file incorrectly
defines a main() function that calls test_split_edge_cases() and
test_empty_pattern(); remove the entire main() function so tests are run by
Rust's #[test] harness instead ( locate and delete the main() definition that
invokes test_split_edge_cases and test_empty_pattern ); rely on the existing
#[test] functions and run them via cargo test.
There was a problem hiding this comment.
Pull request overview
This PR enhances CheetahString with flexible pattern matching support, allowing methods like starts_with, ends_with, contains, and split to accept both character (char) and string (&str) patterns through sealed trait implementations. This provides a more ergonomic API consistent with Rust's standard library string methods.
Key Changes:
- Introduced
StrPatternandSplitPatterntraits with sealed trait pattern to support bothcharand&strpatterns - Implemented custom
SplitStriterator with support for empty pattern splitting (matching standard library behavior) - Enhanced return type of
chars()fromimpl Iteratorto concretestd::str::Chars<'_>to exposeDoubleEndedIteratorfunctionality
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib.rs |
Exports new public types including pattern traits and split iterator wrappers |
src/cheetah_string.rs |
Core implementation of pattern traits, updated method signatures for starts_with, ends_with, contains, and split, plus custom SplitStr iterator with empty pattern support |
tests/patterns.rs |
New comprehensive tests for character and string pattern matching across all affected methods |
tests/split_edge_cases.rs |
Edge case tests for split functionality including empty strings, consecutive separators, and empty patterns |
tests/comprehensive_tests.rs |
Integration tests covering unicode support, iterator behavior, reverse iteration, and special characters |
tests/sso.rs |
Updated Unicode tests from Chinese to Latin extended characters (2-byte UTF-8) |
tests/basic.rs |
Updated Unicode character tests to use Latin extended characters |
tests/api_extensions.rs |
Updated Unicode-related tests with Latin extended characters |
examples/test_split_behavior.rs |
Example demonstrating split behavior parity with standard library for both char and string patterns |
examples/test_empty_pattern.rs |
Example showcasing empty pattern splitting behavior |
examples/pattern_matching.rs |
Example demonstrating pattern matching with chars and strings |
examples/comprehensive_patterns.rs |
Comprehensive examples covering all pattern-related functionality |
Comments suppressed due to low confidence (3)
src/cheetah_string.rs:489
- With the new generic
starts_withmethod that accepts bothcharand&strpatterns, thestarts_with_charmethod has become redundant. Users can now callstarts_with('+')directly instead ofstarts_with_char('+'). Consider adding a deprecation warning to guide users to the more flexible API, or document when and why someone would preferstarts_with_charoverstarts_with.
/// Returns `true` if the string starts with the given character.
///
/// # Examples
///
/// ```
/// use cheetah_string::CheetahString;
///
/// let s = CheetahString::from("hello world");
/// assert!(s.starts_with_char('h'));
/// assert!(!s.starts_with_char('w'));
/// ```
#[inline]
pub fn starts_with_char(&self, pat: char) -> bool {
self.as_str().starts_with(pat)
}
src/cheetah_string.rs:525
- With the new generic
ends_withmethod that accepts bothcharand&strpatterns, theends_with_charmethod has become redundant. Users can now callends_with('d')directly instead ofends_with_char('d'). Consider adding a deprecation warning to guide users to the more flexible API, or document when and why someone would preferends_with_charoverends_with.
/// Returns `true` if the string ends with the given character.
///
/// # Examples
///
/// ```
/// use cheetah_string::CheetahString;
///
/// let s = CheetahString::from("hello world");
/// assert!(s.ends_with_char('d'));
/// assert!(!s.ends_with_char('h'));
/// ```
#[inline]
pub fn ends_with_char(&self, pat: char) -> bool {
self.as_str().ends_with(pat)
}
src/cheetah_string.rs:561
- With the new generic
containsmethod that accepts bothcharand&strpatterns, thecontains_charmethod has become redundant. Users can now callcontains('o')directly instead ofcontains_char('o'). Consider adding a deprecation warning to guide users to the more flexible API, or document when and why someone would prefercontains_charovercontains.
/// Returns `true` if the string contains the given character.
///
/// # Examples
///
/// ```
/// use cheetah_string::CheetahString;
///
/// let s = CheetahString::from("hello world");
/// assert!(s.contains_char('o'));
/// assert!(!s.contains_char('x'));
/// ```
#[inline]
pub fn contains_char(&self, pat: char) -> bool {
self.as_str().contains(pat)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl SplitSealed for &str {} | ||
| } | ||
|
|
||
| /// A pattern that can be used with `starts_with` and `ends_with` methods. |
There was a problem hiding this comment.
The documentation comment for StrPattern says "A pattern that can be used with starts_with and ends_with methods" but this trait is also used by the contains method. The documentation should be updated to include contains in the list of methods that accept this pattern type.
| /// A pattern that can be used with `starts_with` and `ends_with` methods. | |
| /// A pattern that can be used with `starts_with`, `ends_with`, and `contains` methods. |
| impl<'a> DoubleEndedIterator for SplitWrapper<'a> { | ||
| fn next_back(&mut self) -> Option<Self::Item> { | ||
| match self { | ||
| SplitWrapper::Char(iter) => iter.next_back(), | ||
| SplitWrapper::Str(_) => { | ||
| // String pattern split doesn't support reverse iteration | ||
| // This is consistent with std::str::Split<&str> | ||
| panic!("split with string pattern does not support reverse iteration") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The panic message and behavior for reverse iteration on string patterns is intentional and documented, but it may be surprising to users who don't realize the limitation. The panic occurs at runtime rather than being caught at compile time. Consider whether this behavior matches the standard library. Note that in Rust's standard library, str::split with a string pattern returns Split<'_, &str> which does NOT implement DoubleEndedIterator, so calling .rev() would be a compile-time error, not a runtime panic. The current implementation makes this a runtime error by implementing DoubleEndedIterator unconditionally on SplitWrapper. This is a behavioral difference from the standard library that could lead to unexpected panics in user code.
| mod serde; | ||
|
|
||
| pub use cheetah_string::CheetahString; | ||
| pub use cheetah_string::{CheetahString, SplitPattern, SplitStr, SplitWrapper, StrPattern}; |
There was a problem hiding this comment.
The types SplitPattern, SplitStr, and SplitWrapper are implementation details and should not be part of the public API. They are not used directly by any tests or examples, and exposing them increases the API surface area unnecessarily, making it harder to change the implementation in the future. Consider removing them from the public exports and keeping only CheetahString and StrPattern (if StrPattern needs to be public for generic code to work).
| pub use cheetah_string::{CheetahString, SplitPattern, SplitStr, SplitWrapper, StrPattern}; | |
| pub use cheetah_string::{CheetahString, StrPattern}; |
| #[test] | ||
| fn test_empty_pattern() { | ||
| // Empty pattern should split between each character | ||
| let s = CheetahString::from("hello"); | ||
| let parts: Vec<&str> = s.split("").collect(); | ||
| println!("Empty pattern: {:?}", parts); | ||
| // Standard library behavior: empty pattern splits between each character | ||
| } |
There was a problem hiding this comment.
The test test_empty_pattern doesn't have any assertions to verify the behavior. The comment mentions "Standard library behavior: empty pattern splits between each character" but there's no actual assertion to check if the implementation matches this behavior. Add an assertion to verify the expected behavior, for example: assert_eq!(parts, vec!["", "h", "e", "l", "l", "o", ""]); to match standard library behavior.
|
|
||
| fn main() { | ||
| test_split_edge_cases(); | ||
| test_empty_pattern(); | ||
| println!("\nAll tests passed!"); | ||
| } |
There was a problem hiding this comment.
The main function in a test file is unusual. Test files should only contain test functions marked with #[test] or test modules marked with #[cfg(test)]. The main function will not be executed by cargo test and serves no purpose here. Consider removing it.
| fn main() { | |
| test_split_edge_cases(); | |
| test_empty_pattern(); | |
| println!("\nAll tests passed!"); | |
| } |
Fix #92
Summary by CodeRabbit
Release Notes
New Features
starts_with,ends_with, andcontainsnow accept both character and string patterns for greater flexibility.splitfunctionality with flexible pattern support and double-ended iteration capabilities.Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.