added Selection::ancestors
#28
Conversation
WalkthroughThe changes introduced in this pull request enhance the documentation and functionality of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DOM
participant Selection
User->>DOM: Parse Document
DOM->>Selection: Create Selection
User->>Selection: Select Elements
User->>Selection: Get Ancestors
Selection->>DOM: Retrieve Ancestors
User->>Selection: Manipulate Attributes
Selection->>DOM: Update Attributes
User->>DOM: Serialize to HTML
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
+ Coverage 83.20% 83.30% +0.09%
==========================================
Files 13 13
Lines 1989 2000 +11
==========================================
+ Hits 1655 1666 +11
Misses 334 334 ☔ 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: 0
🧹 Outside diff range and nitpick comments (6)
tests/selection-traversal.rs (2)
Line range hint
312-331: LGTM! Consider enhancing the test documentation.The test effectively verifies the full ancestor chain traversal functionality. The assertions appropriately check both presence and absence of nodes in the ancestor chain.
Consider adding a comment explaining why we test
!ancestor_sel.is("#first-child")to make it clearer that we're verifying the ancestor chain excludes the starting node.
Line range hint
362-370: Consider enhancing iterator test coverage.While the test verifies the count of ancestors, consider adding assertions to verify the actual content/order of the iterator results.
let child_node = child_sel.nodes().first().unwrap(); -let ancestors = child_node.ancestors_it(Some(2)); -// utilizing ancestors iterator (without intermediate collection) -// got 2 ancestors -assert!(ancestors.count() == 2); +let mut ancestors = child_node.ancestors_it(Some(2)); +// Verify both count and content of ancestors +let first = ancestors.next().unwrap(); +let second = ancestors.next().unwrap(); +assert!(ancestors.next().is_none()); // Verify we got exactly 2 +assert_eq!(first.id(), Some("parent")); +assert_eq!(second.id(), Some("grand-parent"));Examples.md (2)
231-233: Consider improving the comment's clarityThe comment provides a useful shorthand alternative, but could be more explicit about what it's simplifying. Consider revising to better explain the relationship with the previous code.
- -// or just: let ancestor_sel = child_sel.ancestors(None); - +// Shorthand alternative to the above two lines: +// Instead of creating Selection from ancestors, you can directly call: +// let ancestor_sel = child_sel.ancestors(None);
231-233: Consider enhancing the ancestors example sectionThe example thoroughly demonstrates the API, but could be more helpful with:
- A brief introduction explaining when and why you'd use ancestor selection
- Common use cases (e.g., finding closest parent with specific class, traversing up to specific ancestor)
- Performance considerations when using
max_limitvs no limitConsider adding this introductory text at the start of the section:
// Ancestor selection allows you to traverse up the DOM tree from a selected element. // This is useful for: // - Finding the closest parent matching a condition // - Checking if an element is contained within a specific ancestor // - Building breadcrumb navigation // // The `ancestors()` method supports both unlimited traversal (None) and // limited traversal (Some(n)) for performance optimization.README.md (1)
264-265: LGTM! Consider highlighting the alternative approach more prominently.The added comment effectively documents the alternative, more concise approach using
child_sel.ancestors(None). This is valuable information for users.Consider restructuring to make the alternative approach more prominent, as it's likely the preferred method:
-// or just: let ancestor_sel = child_sel.ancestors(None); +// Alternative, more concise approach: +// let ancestor_sel = child_sel.ancestors(None);src/selection.rs (1)
531-554: LGTM! Consider enhancing documentation with usage examples.The implementation looks solid and follows the established patterns in the codebase. The method correctly handles duplicate prevention and properly filters for element nodes.
Consider adding usage examples to the documentation, similar to other methods in the codebase. For example:
/// # Examples /// /// ``` /// # use dom_query::Document; /// let doc = Document::from("<div><p><span>Hello</span></p></div>"); /// let span = doc.select("span").first(); /// /// // Get all ancestors /// let all_ancestors = span.ancestors(None); /// assert_eq!(all_ancestors.length(), 2); // matches p and div /// /// // Get ancestors with depth limit /// let limited_ancestors = span.ancestors(Some(1)); /// assert_eq!(limited_ancestors.length(), 1); // matches only p /// ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
Examples.md(1 hunks)README.md(1 hunks)src/selection.rs(1 hunks)tests/data.rs(1 hunks)tests/node-manipulation.rs(2 hunks)tests/selection-traversal.rs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/node-manipulation.rs
🔇 Additional comments (4)
tests/data.rs (1)
23-24: LGTM! Well-structured test data changes.
The modification to split the single child into two siblings with semantic IDs (first-child and second-child) is clean and properly formatted. This change effectively supports testing ancestor selection functionality for multiple elements.
Let's verify the HTML validity:
tests/selection-traversal.rs (2)
Line range hint 339-353: LGTM! Well-structured test for limited ancestor traversal.
The test effectively verifies the limited ancestor traversal functionality, with good coverage of both included and excluded ancestors.
372-388: LGTM! Excellent test coverage for multi-node ancestor selection.
The test effectively verifies that the Selection::ancestors method works correctly when finding common ancestors of multiple nodes, with appropriate limit handling.
src/selection.rs (1)
555-555: LGTM! Deprecation follows best practices.
The deprecation of next in favor of next_sibling is well-handled with a clear migration path for users.
Summary by CodeRabbit