fixed Tree::nodes visibility
#33
Conversation
WalkthroughThe pull request updates 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
=======================================
Coverage 84.07% 84.07%
=======================================
Files 13 13
Lines 2122 2122
=======================================
Hits 1784 1784
Misses 338 338 ☔ 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 (3)
CHANGELOG.md (1)
6-10: Consider adding migration notes for the visibility change.The changelog entry correctly documents the visibility fix. However, since this is a breaking change that could affect downstream users who were directly accessing
Tree::nodes, consider adding migration notes to help users update their code.Add migration guidance like:
### Fixed - Hide visibility of `Tree::nodes`, which was accidentally exposed in the previous version. + +### Migration +- If you were directly accessing `Tree::nodes`, use the public methods provided by the `Tree` struct instead.src/dom_tree.rs (2)
Line range hint
300-330: Add documentation about behavior when parent node doesn't exist.The method should document its behavior when the parent node specified by
iddoesn't exist in the tree.Add this documentation:
/// Creates a new element from data and appends it to a node by id + /// + /// # Arguments + /// * `id` - The id of the parent node. If the node doesn't exist, the operation will have no effect. + /// * `data` - The data for the new node pub fn append_child_data_of(&self, id: &NodeId, data: NodeData) {
Line range hint
332-367: Improve error handling in append_child_of and prepend_child_of.Both methods have TODOs about panic behavior and silently return when the parent node doesn't exist. Consider:
- Using Result type to propagate errors
- Sharing common code between these similar methods
Consider refactoring to:
#[derive(Debug)] pub enum TreeError { NodeNotFound(NodeId), } impl Tree { fn append_child_of(&self, id: &NodeId, new_child_id: &NodeId) -> Result<(), TreeError> { let mut nodes = self.nodes.borrow_mut(); let parent = nodes.get_mut(id.value) .ok_or_else(|| TreeError::NodeNotFound(*id))?; // ... rest of the implementation Ok(()) } fn prepend_child_of(&self, id: &NodeId, new_child_id: &NodeId) -> Result<(), TreeError> { let mut nodes = self.nodes.borrow_mut(); let parent = nodes.get_mut(id.value) .ok_or_else(|| TreeError::NodeNotFound(*id))?; // ... rest of the implementation Ok(()) } }Also applies to: 369-404
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)Cargo.toml(1 hunks)src/dom_tree.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🔇 Additional comments (2)
CHANGELOG.md (1)
5-5: LGTM!
The spacing and formatting between changelog entries is consistent with the rest of the file.
Also applies to: 11-11
src/dom_tree.rs (1)
24-24: LGTM! Good encapsulation of internal tree structure.
Changing nodes field visibility to pub(crate) is a positive change that:
- Prevents external code from directly manipulating the tree's internal state
- Enables future internal refactoring without breaking external code
- Ensures tree consistency by forcing all modifications through the public API
Summary by CodeRabbit
Bug Fixes
Tree::nodesfield.Documentation
0.9.1, including enhancements and fixes from previous versions.