src/node/serializing.rs: implement format_text
#62
Conversation
|
Warning Rate limit exceeded@niklak has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe changes add new text formatting functionality across multiple components. New 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 #62 +/- ##
==========================================
+ Coverage 91.90% 92.12% +0.22%
==========================================
Files 16 16
Lines 2618 2706 +88
==========================================
+ Hits 2406 2493 +87
- Misses 212 213 +1 ☔ 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: 1
🧹 Nitpick comments (3)
src/node/serializing.rs (1)
183-207: Consider edge cases in text normalization.The
normalize_textfunction handles basic whitespace normalization well, but consider adding documentation about its behavior with:
- Unicode whitespace characters
- Zero-width spaces
- Non-breaking spaces
fn normalize_text(text: &str, follows_newline: bool) -> StrTendril { + // Handle all Unicode whitespace characters including: + // - Regular spaces + // - Non-breaking spaces + // - Zero-width spaces + // - Other Unicode whitespace let push_start_whitespace = !follows_newline && text.starts_with(char::is_whitespace);tests/node-traversal.rs (1)
332-353: Consider adding more test cases.While the current test is good, consider adding test cases for:
- Empty documents
- Documents with only whitespace
- Documents with special HTML elements (pre, br, etc.)
- Documents with nested formatting
Here's a suggested additional test case:
#[cfg_attr(not(target_arch = "wasm32"), test)] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn test_doc_formatted_text_edge_cases() { // Test empty document let doc = Document::from(""); assert_eq!(doc.formatted_text().as_ref(), ""); // Test whitespace-only document let doc = Document::from(" \n \t "); assert_eq!(doc.formatted_text().as_ref(), ""); // Test pre tag formatting let doc = Document::from("<pre> preserved whitespace </pre>"); assert_eq!(doc.formatted_text().as_ref(), " preserved whitespace \n\n"); }src/document.rs (1)
120-129: Add examples to the documentation.The documentation clearly explains the formatting rules but would benefit from examples showing the input and expected output.
Add examples like this:
/// Returns the formatted text of the document and its descendants. This is the same as /// the `text()` method, but with a few differences: /// /// - Whitespace is normalized so that there is only one space between words. /// - All whitespace is removed from the beginning and end of the text. /// - After block elements, a double newline is added. /// - For elements like `br`, 'hr', 'li', 'tr' a single newline is added. +/// +/// # Examples +/// +/// ``` +/// use dom_query::Document; +/// +/// let doc = Document::from(r#"<div>Hello World</div><p>First</p><p>Second</p>"#); +/// assert_eq!(doc.formatted_text(), "Hello World\n\nFirst\n\nSecond\n\n"); +/// ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)src/document.rs(1 hunks)src/node.rs(1 hunks)src/node/node_ref.rs(2 hunks)src/node/serializing.rs(2 hunks)src/selection.rs(2 hunks)tests/data.rs(1 hunks)tests/node-traversal.rs(2 hunks)tests/selection-traversal.rs(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Rust Clippy & Audit
src/node/node_ref.rs
[error] 562-562: Empty line after doc comment.
🪛 GitHub Check: codecov/patch
src/node/serializing.rs
[warning] 108-108: src/node/serializing.rs#L108
Added line #L108 was not covered by tests
[warning] 114-114: src/node/serializing.rs#L114
Added line #L114 was not covered by tests
[warning] 124-125: src/node/serializing.rs#L124-L125
Added lines #L124 - L125 were not covered by tests
[warning] 134-137: src/node/serializing.rs#L134-L137
Added lines #L134 - L137 were not covered by tests
[warning] 169-170: src/node/serializing.rs#L169-L170
Added lines #L169 - L170 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (6)
src/node.rs (1)
18-18: LGTM!The visibility modifier
pub(crate)is appropriate for theformat_textfunction as it should only be accessible within the crate.tests/data.rs (1)
108-128: LGTM!The test data is well-structured and includes various text formatting scenarios that will help validate the text formatting functionality.
src/node/serializing.rs (1)
90-181: Add test coverage for untested code paths.The following code paths need test coverage:
- Empty text content handling (line 114)
- Pre tag content handling (lines 124-125)
- Document/Fragment node handling (lines 134-137)
- Line break handling for br/hr/li/tr tags (lines 169-170)
Let's verify the untested code paths:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-108: src/node/serializing.rs#L108
Added line #L108 was not covered by tests
[warning] 114-114: src/node/serializing.rs#L114
Added line #L114 was not covered by tests
[warning] 124-125: src/node/serializing.rs#L124-L125
Added lines #L124 - L125 were not covered by tests
[warning] 134-137: src/node/serializing.rs#L134-L137
Added lines #L134 - L137 were not covered by tests
[warning] 169-170: src/node/serializing.rs#L169-L170
Added lines #L169 - L170 were not covered by teststests/selection-traversal.rs (1)
527-551: LGTM!The test case is well-structured and thoroughly validates the text formatting rules, including whitespace normalization and newline handling.
src/selection.rs (1)
180-193: LGTM!The implementation correctly handles multiple selected nodes by concatenating their formatted text while maintaining consistent formatting rules.
CHANGELOG.md (1)
9-10: Changelog Entry for New Formatted Text MethodsThe updated changelog clearly documents the addition of the new formatted text methods:
Document::formatted_textSelection::formatted_textNodeRef::formatted_textThis entry aligns well with the PR objectives and provides a concise description of the new functionality. Consider if additional context—such as a note on underlying normalization behavior or reference to the internal helper functions—would add value for users looking for more granular details.
src/node/node_ref.rs
Outdated
| /// Returns the formatted text of the node and its descendants. This is the same as | ||
| /// the `text()` method, but with a few differences: | ||
| /// | ||
| /// - Whitespace is normalized so that there is only one space between words. | ||
| /// - All whitespace is removed from the beginning and end of the text. | ||
| /// - After block elements, a double newline is added. | ||
| /// - For elements like `br`, 'hr', 'li', 'tr' a single newline is added. | ||
|
|
||
| pub fn formatted_text(&self) -> StrTendril { | ||
| format_text(self, false) | ||
| } |
There was a problem hiding this comment.
Fix the empty line after doc comment.
Remove the empty line after the doc comment to fix the Clippy warning.
Apply this diff:
/// Returns the formatted text of the node and its descendants. This is the same as
/// the `text()` method, but with a few differences:
///
/// - Whitespace is normalized so that there is only one space between words.
/// - All whitespace is removed from the beginning and end of the text.
/// - After block elements, a double newline is added.
/// - For elements like `br`, 'hr', 'li', 'tr' a single newline is added.
-
pub fn formatted_text(&self) -> StrTendril {
format_text(self, false)
}📝 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.
| /// Returns the formatted text of the node and its descendants. This is the same as | |
| /// the `text()` method, but with a few differences: | |
| /// | |
| /// - Whitespace is normalized so that there is only one space between words. | |
| /// - All whitespace is removed from the beginning and end of the text. | |
| /// - After block elements, a double newline is added. | |
| /// - For elements like `br`, 'hr', 'li', 'tr' a single newline is added. | |
| pub fn formatted_text(&self) -> StrTendril { | |
| format_text(self, false) | |
| } | |
| /// Returns the formatted text of the node and its descendants. This is the same as | |
| /// the `text()` method, but with a few differences: | |
| /// | |
| /// - Whitespace is normalized so that there is only one space between words. | |
| /// - All whitespace is removed from the beginning and end of the text. | |
| /// - After block elements, a double newline is added. | |
| /// - For elements like `br`, 'hr', 'li', 'tr' a single newline is added. | |
| pub fn formatted_text(&self) -> StrTendril { | |
| format_text(self, false) | |
| } |
🧰 Tools
🪛 GitHub Actions: Rust Clippy & Audit
[error] 562-562: Empty line after doc comment.
Document::formatted_text,Selection::formatted_text, andNodeRef::formatted_text, which return formatted text of the document, selection, or node respectively.