added Selection::add_selection and other add methods
#31
Conversation
WalkthroughThis pull request introduces substantial updates to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 83.40% 83.59% +0.18%
==========================================
Files 13 13
Lines 2001 2060 +59
==========================================
+ Hits 1669 1722 +53
- Misses 332 338 +6 ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (5)
CHANGELOG.md (1)
18-18: Consider rewording to avoid repetitive sentence beginnings.The three consecutive sentences starting with "Added" could be rewritten for better readability.
-- Added `Selection::add_selection`, `Selection:add_matcher`, `Selection::add` and `Selection::try_add` methods to extend current selection with other selections. +Introduced new selection methods: `Selection::add_selection`, `Selection:add_matcher`, `Selection::add` and `Selection::try_add` to extend current selection with other selections.🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - AddedSelection::add_selection, `Selection:...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
tests/selection-traversal.rs (3)
390-401: Consider adding edge cases to test_selection_add_selection.While the test covers the basic functionality, consider adding test cases for:
- Combining empty selections
- Adding a selection to itself (checking for duplicates)
- Adding selections with overlapping nodes
403-410: Enhance test coverage with complex selectors.Consider adding test cases with more complex selectors to verify the robustness of the add method:
- Multiple selectors (e.g., "div, span")
- Nested selectors (e.g., "div > span")
- Attribute selectors
- Pseudo-classes
421-434: Add more edge cases to try_add testing.The test covers basic success and failure cases well. Consider adding:
- Test with empty string selector
- Test with whitespace-only selector
- Test adding multiple selectors in one call
- Test with selector that matches no elements
src/selection.rs (1)
305-305: Increase test coverage for new methodsStatic analysis indicates that certain branches in the new methods are not covered by tests (lines 305, 324, 340, 362, 733). Enhancing test coverage for these lines will improve code reliability and catch potential issues early.
Would you like assistance in creating unit tests to cover these cases?
Also applies to: 324-324, 340-340, 362-362, 733-733
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 305-305: src/selection.rs#L305
Added line #L305 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)src/dom_tree.rs(1 hunks)src/selection.rs(2 hunks)tests/node-traversal.rs(1 hunks)tests/pseudo-classes.rs(0 hunks)tests/selection-traversal.rs(1 hunks)
💤 Files with no reviewable changes (1)
- tests/pseudo-classes.rs
✅ Files skipped from review due to trivial changes (1)
- src/dom_tree.rs
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - Added Selection::add_selection, `Selection:...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 GitHub Check: codecov/patch
src/selection.rs
[warning] 305-305: src/selection.rs#L305
Added line #L305 was not covered by tests
[warning] 324-324: src/selection.rs#L324
Added line #L324 was not covered by tests
[warning] 340-340: src/selection.rs#L340
Added line #L340 was not covered by tests
[warning] 362-362: src/selection.rs#L362
Added line #L362 was not covered by tests
[warning] 733-733: src/selection.rs#L733
Added line #L733 was not covered by tests
🔇 Additional comments (5)
tests/node-traversal.rs (2)
Line range hint 13-55: LGTM! Well-structured test cases with comprehensive edge case coverage.
The test cases effectively verify the first_element_child behavior across different HTML structures:
- Empty elements
- Text-only elements
- Multiple children
- Nested elements
The whitespace improvements enhance readability by clearly separating different test scenarios.
51-55: Nice formatting improvement for better readability.
The multi-line assertion formatting enhances code readability while maintaining the same test logic.
CHANGELOG.md (3)
Line range hint 28-44: Well-structured documentation of breaking changes.
The version 0.8.0 changes are clearly documented with appropriate categorization of changes, additions, and removals. The rationale for changes (e.g., "simplifying code and improving readability") is helpful for users upgrading to this version.
🧰 Tools
🪛 LanguageTool
[style] ~16-~16: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - Added NodeRef::prepend_html method, that pa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - Added Selection::prepend_html method, which...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - Added Selection::add_selection, `Selection:...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[duplication] ~20-~20: Possible typo: you repeated a word
Context: ...t selection with other selections. ### Fixed - Fixed Selection::append_selection to work w...
(ENGLISH_WORD_REPEAT_RULE)
Line range hint 1-200: Excellent changelog structure and maintenance.
The changelog follows best practices with:
- Clear version numbering and dates
- Consistent categorization (Changed, Added, Fixed, Removed)
- Explicit marking of breaking changes
- Detailed explanations of significant changes
🧰 Tools
🪛 LanguageTool
[style] ~16-~16: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - Added NodeRef::prepend_html method, that pa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - Added Selection::prepend_html method, which...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - Added Selection::add_selection, `Selection:...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[duplication] ~20-~20: Possible typo: you repeated a word
Context: ...t selection with other selections. ### Fixed - Fixed Selection::append_selection to work w...
(ENGLISH_WORD_REPEAT_RULE)
Line range hint 22-26: Verify impact of the critical NodeRef::first_element_child fix.
The documented fix for NodeRef::first_element_child indicates a significant behavior change that could affect DOM traversal. Let's verify the impact across the codebase.
Would you like me to help create additional test cases to verify the fixed behavior?
✅ Verification successful
Let me analyze the test coverage for this critical fix by examining the test cases in detail.
The first_element_child fix is well-verified with comprehensive test coverage
The implementation and test coverage analysis shows:
- The fixed implementation correctly iterates through child nodes to find the first element child
- Tests cover all critical edge cases:
- Empty elements (no children)
- Text-only nodes
- Multiple children scenarios
- Nested element structures
The fix is properly implemented and thoroughly tested, ensuring the correct behavior for DOM traversal operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of first_element_child to identify potentially affected code
# Test: Find direct method calls
echo "Direct method calls:"
rg "first_element_child" -A 2
# Test: Find test files that might need updates
echo "\nRelated test files:"
fd -e rs -e test
Length of output: 2645
Script:
#!/bin/bash
# Check the implementation and test coverage
echo "Implementation in node_ref.rs:"
rg -A 10 "pub fn first_element_child" src/node/node_ref.rs
echo "\nTest cases in node-traversal.rs:"
cat tests/node-traversal.rs
Length of output: 2285
🧰 Tools
🪛 LanguageTool
[style] ~16-~16: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - Added NodeRef::prepend_html method, that pa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - Added Selection::prepend_html method, which...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - Added Selection::add_selection, `Selection:...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[duplication] ~20-~20: Possible typo: you repeated a word
Context: ...t selection with other selections. ### Fixed - Fixed Selection::append_selection to work w...
(ENGLISH_WORD_REPEAT_RULE)
| #[cfg_attr(not(target_arch = "wasm32"), test)] | ||
| #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] | ||
| #[should_panic] | ||
| fn test_selection_add_panic() { | ||
| let doc: Document = ANCESTORS_CONTENTS.into(); | ||
|
|
||
| doc.select("#first-child").add(":;'"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve panic testing specificity.
Instead of using a broad #[should_panic], consider using #[should_panic(expected = "...")] to verify the specific error message. This ensures that the test fails for the expected reason rather than any panic.
-#[should_panic]
+#[should_panic(expected = "failed to parse selector")]
fn test_selection_add_panic() {Committable suggestion skipped: line range outside the PR's diff.
| fn ensure_same_tree(&self, other: &Selection) { | ||
| let tree = self.nodes().first().unwrap().tree; | ||
| let other_tree = other.nodes().first().unwrap().tree; | ||
| if !std::ptr::eq(tree, other_tree) { | ||
| panic!("Selections must be from the same tree"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential panic when selections are empty in ensure_same_tree
The method ensure_same_tree may panic when either self or other is empty because it calls unwrap() on first() without checking if there are any nodes. This can lead to a runtime panic. Consider adding checks for empty selections before accessing self.nodes().first().unwrap() and handle the empty case appropriately.
Apply this diff to handle empty selections:
fn ensure_same_tree(&self, other: &Selection) {
+ if self.is_empty() || other.is_empty() {
+ panic!("Selections must not be empty");
+ }
let tree = self.nodes().first().unwrap().tree;
let other_tree = other.nodes().first().unwrap().tree;
if !std::ptr::eq(tree, other_tree) {
panic!("Selections must be from the same tree");
}
}📝 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 ensure_same_tree(&self, other: &Selection) { | |
| let tree = self.nodes().first().unwrap().tree; | |
| let other_tree = other.nodes().first().unwrap().tree; | |
| if !std::ptr::eq(tree, other_tree) { | |
| panic!("Selections must be from the same tree"); | |
| } | |
| } | |
| fn ensure_same_tree(&self, other: &Selection) { | |
| if self.is_empty() || other.is_empty() { | |
| panic!("Selections must not be empty"); | |
| } | |
| let tree = self.nodes().first().unwrap().tree; | |
| let other_tree = other.nodes().first().unwrap().tree; | |
| if !std::ptr::eq(tree, other_tree) { | |
| panic!("Selections must be from the same tree"); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 733-733: src/selection.rs#L733
Added line #L733 was not covered by tests
There was a problem hiding this comment.
I pushed a fix in commit 88dfbd7, please review it.
There was a problem hiding this comment.
@niklak, thank you for fixing the issue. I have reviewed the changes in commit 88dfbd7, and the added checks in ensure_same_tree now properly handle empty selections, preventing potential panics. Great job!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| pub fn add_matcher(&self, matcher: &Matcher) -> Selection<'a> { | ||
| if self.is_empty() { | ||
| return self.clone(); | ||
| } | ||
| let root = self.nodes().first().unwrap().tree.root(); | ||
| let other_nodes: Vec<NodeRef> = | ||
| Matches::from_one(root, matcher, MatchScope::IncludeNode).collect(); | ||
| let new_nodes = self.merge_nodes(other_nodes); | ||
| Selection { nodes: new_nodes } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle empty self in add_matcher to include matches from the entire document
Currently, add_matcher returns an empty selection when self is empty, which might not be the expected behavior. When the current selection is empty, it may be more intuitive to perform the addition starting from the document root. Consider modifying add_matcher to handle the empty self case by initiating the search from the document root.
Proposed change:
pub fn add_matcher(&self, matcher: &Matcher) -> Selection<'a> {
- if self.is_empty() {
- return self.clone();
- }
- let root = self.nodes().first().unwrap().tree.root();
+ let root = if self.is_empty() {
+ // If self is empty, start from the root of the tree
+ self.nodes().first().map_or_else(
+ || matcher.document_root(),
+ |node| node.tree.root()
+ )
+ } else {
+ self.nodes().first().unwrap().tree.root()
+ };
let other_nodes: Vec<NodeRef> =
Matches::from_one(root, matcher, MatchScope::IncludeNode).collect();
let new_nodes = self.merge_nodes(other_nodes);
Selection { nodes: new_nodes }
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 340-340: src/selection.rs#L340
Added line #L340 was not covered by tests
There was a problem hiding this comment.
@niklak, understood.
✏️ Learnings added
Learnt from: niklak
PR: niklak/dom_query#31
File: src/selection.rs:338-347
Timestamp: 2024-11-08T15:39:07.660Z
Learning: In `Selection::add_matcher` within `src/selection.rs`, when `self` is empty, it should return an empty selection instead of starting from the document root.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
src/selection.rs
Outdated
| pub fn add_selection(&self, other: &'a Selection) -> Selection<'a> { | ||
| if other.is_empty() { | ||
| return self.clone(); | ||
| } | ||
|
|
||
| self.ensure_same_tree(other); | ||
|
|
||
| let other_nodes = other.nodes(); | ||
| let new_nodes = self.merge_nodes(other_nodes.to_vec()); | ||
|
|
||
| Selection { nodes: new_nodes } | ||
| } |
There was a problem hiding this comment.
Possible panic in add_selection when self is empty
In the method add_selection, if self is empty and other is not, calling self.ensure_same_tree(other) will cause a panic due to unwrap() on self.nodes().first(). Consider adding a check for self.is_empty() before calling ensure_same_tree to prevent potential panics.
Apply this diff to handle the case when self is empty:
pub fn add_selection(&self, other: &'a Selection) -> Selection<'a> {
+ if self.is_empty() {
+ return other.clone();
+ }
if other.is_empty() {
return self.clone();
}
self.ensure_same_tree(other);
let other_nodes = other.nodes();
let new_nodes = self.merge_nodes(other_nodes.to_vec());
Selection { nodes: new_nodes }
}📝 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.
| pub fn add_selection(&self, other: &'a Selection) -> Selection<'a> { | |
| if other.is_empty() { | |
| return self.clone(); | |
| } | |
| self.ensure_same_tree(other); | |
| let other_nodes = other.nodes(); | |
| let new_nodes = self.merge_nodes(other_nodes.to_vec()); | |
| Selection { nodes: new_nodes } | |
| } | |
| pub fn add_selection(&self, other: &'a Selection) -> Selection<'a> { | |
| if self.is_empty() { | |
| return other.clone(); | |
| } | |
| if other.is_empty() { | |
| return self.clone(); | |
| } | |
| self.ensure_same_tree(other); | |
| let other_nodes = other.nodes(); | |
| let new_nodes = self.merge_nodes(other_nodes.to_vec()); | |
| Selection { nodes: new_nodes } | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 362-362: src/selection.rs#L362
Added line #L362 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
CHANGELOG.md (3)
14-18: Improve readability by varying sentence structure.Consider rewording these entries to avoid repetitive sentence beginnings while maintaining clarity:
-Implemented `NodeRef::prepend_child` method, that inserts a child at the beginning of node content. -Implemented `NodeRef::prepend_children` method, that inserts a child and its siblings at the beginning of the node content. -Implemented `NodeRef::prepend_html` method, that parses html string and inserts its parsed nodes at the beginning of the node content. -Implemented `Selection::prepend_html` method, which parses an HTML string and inserts its parsed nodes at the beginning of the content of all matched nodes. -Implemented `Selection::add_selection`, `Selection:add_matcher`, `Selection::add` and `Selection::try_add` methods to extend current selection with other selections. +New `NodeRef` methods: +- `prepend_child`: Inserts a child at the beginning of node content +- `prepend_children`: Inserts a child and its siblings at the beginning of node content +- `prepend_html`: Parses HTML string and prepends the resulting nodes + +New `Selection` methods: +- `prepend_html`: Parses and prepends HTML content to all matched nodes +- `add_selection`, `add_matcher`, `add`, `try_add`: Extend current selection with additional nodes🧰 Tools
🪛 LanguageTool
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - ImplementedSelection::prepend_htmlmethod, which...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - ImplementedSelection::add_selection, `Selection:...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Line range hint
1-24: Maintain consistent formatting in changelog entries.The changelog entries should follow consistent formatting rules:
- End all bullet points either with or without periods (currently mixed)
- Maintain consistent detail level across similar entries
- Group related changes under appropriate sections
Consider updating the style guide to establish these conventions for future entries.
🧰 Tools
🪛 LanguageTool
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - ImplementedSelection::prepend_htmlmethod, which...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - ImplementedSelection::add_selection, `Selection:...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[duplication] ~20-~20: Possible typo: you repeated a word
Context: ...t selection with other selections. ### Fixed - FixedSelection::append_selectionto work w...(ENGLISH_WORD_REPEAT_RULE)
Line range hint
1-24: Enhance changelog documentation.Consider these improvements to make the changelog more useful:
- Add links to related GitHub issues/PRs for each significant change
- Include migration guides for breaking changes
- Add examples for new features, especially the new selection methods
Example format:
### Added - New selection methods ([#31](link-to-pr)) - `Selection::add_selection`: Extends selection with another selection - Example: ```rust let mut sel1 = doc.select(".class1"); let sel2 = doc.select(".class2"); sel1.add_selection(sel2); ```🧰 Tools
🪛 LanguageTool
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - ImplementedSelection::prepend_htmlmethod, which...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - ImplementedSelection::add_selection, `Selection:...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[duplication] ~20-~20: Possible typo: you repeated a word
Context: ...t selection with other selections. ### Fixed - FixedSelection::append_selectionto work w...(ENGLISH_WORD_REPEAT_RULE)
src/selection.rs (1)
379-385: Optimizemerge_nodesby using aHashSetfor node ID lookupsThe current implementation collects node IDs into a
Vecand checks for their existence usingm.contains(&node.id.value), which results in O(n) lookup time within a loop. This leads to an overall O(n²) time complexity when merging nodes.Consider using a
HashSetformto reduce the lookup time to O(1), significantly improving performance, especially with large selections.Apply this diff to optimize the method:
+use std::collections::HashSet; fn merge_nodes(&self, other_nodes: Vec<NodeRef<'a>>) -> Vec<NodeRef<'a>> { - let m: Vec<usize> = self.nodes().iter().map(|node| node.id.value).collect(); + let m: HashSet<usize> = self.nodes().iter().map(|node| node.id.value).collect(); let add_nodes: Vec<NodeRef> = other_nodes .iter() .filter(|&node| !m.contains(&node.id.value)) .cloned() .collect(); let mut new_nodes = self.nodes().to_vec(); new_nodes.extend(add_nodes); new_nodes }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)src/selection.rs(2 hunks)tests/selection-traversal.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/selection-traversal.rs
🧰 Additional context used
📓 Learnings (1)
src/selection.rs (1)
Learnt from: niklak
PR: niklak/dom_query#31
File: src/selection.rs:338-347
Timestamp: 2024-11-08T15:39:07.860Z
Learning: In `Selection::add_matcher` within `src/selection.rs`, when `self` is empty, it should return an empty selection instead of starting from the document root.
🪛 LanguageTool
CHANGELOG.md
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - Implemented Selection::prepend_html method, which...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - Implemented Selection::add_selection, `Selection:...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (3)
src/selection.rs (3)
289-309: Methods add and try_add are implemented correctly
The implementations of add and try_add methods are sound and follow the expected patterns, correctly handling empty selections and invalid selectors.
338-347: add_matcher method correctly handles empty selections
The add_matcher method appropriately returns a clone of self when the selection is empty, aligning with the established behavior discussed in previous learnings.
360-376: add_selection method properly combines selections
The add_selection method correctly handles scenarios where either selection is empty and ensures that both selections are from the same tree before merging, preventing potential panics.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
CHANGELOG.md (1)
14-18: Consider adding example usage for complex selection methods.While the documentation clearly describes the new methods, the
Selection::add_*methods (particularlyadd_selectionandadd_matcher) could benefit from brief example usage snippets to illustrate their behavior and differences.Example format suggestion:
- Introduced new selection methods: - `Selection::add_selection`: Extends current selection with another selection ```rust let combined = selection1.add_selection(&selection2); ``` - `Selection::add_matcher`: Extends selection using a matcher ```rust let extended = selection.add_matcher(Matcher::new("div.class")); ```🧰 Tools
🪛 LanguageTool
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - Introduced newSelection::prepend_htmlmethod, w...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - Introduced new selection methods: `Selection::add_...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
tests/selection-traversal.rs (3)
390-401: Consider adding edge cases to the test.While the test covers basic functionality well, consider adding test cases for:
- Adding an empty selection
- Adding a selection to itself
- Adding multiple selections in chain
403-410: Enhance selector test coverage.Consider adding test cases with:
- Complex CSS selectors (e.g., combinators, pseudo-classes)
- Multiple selector strings in one call
- Selectors that match no elements
421-434: Expand error handling test coverage.The test covers basic success and failure cases well. Consider adding:
- Tests for chained
try_addcalls usingand_then- More error cases (empty selector, malformed selectors)
- Test with selectors that match no elements (should return Some with length 0)
Example of chained calls:
let result = first_child_sel .try_add("#second-child") .and_then(|sel| sel.try_add("#third-child")); assert!(result.is_some()); assert_eq!(result.unwrap().length(), 3);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)tests/selection-traversal.rs(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - Introduced new Selection::prepend_html method, w...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - Introduced new selection methods: `Selection::add_...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint 1-24: Well-structured changelog following best practices!
The changelog effectively follows the Keep a Changelog format with:
- Clear categorization of changes
- Detailed descriptions, especially for critical fixes
- Proper semantic versioning
- Consistent formatting across versions
🧰 Tools
🪛 LanguageTool
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the beginning of the node content. - Introduced new Selection::prepend_html method, w...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of the content of all matched nodes. - Introduced new selection methods: `Selection::add_...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[duplication] ~19-~19: Possible typo: you repeated a word
Context: ...nt selection with other selections. ### Fixed - Fixed Selection::append_selection to work w...
(ENGLISH_WORD_REPEAT_RULE)
Summary by CodeRabbit
New Features
Tests