refactor: switch to quick_xml library
#101
Conversation
- deserialize only what is needed from XML. `length` and `value` fields are not used and are now ignored when parsing XML - read file contents only once when translating byte offset to line and columns
2bndy5
added
dependencies
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
- Coverage 97.84% 97.58% -0.27%
==========================================
Files 14 14
Lines 3474 3430 -44
==========================================
- Hits 3399 3347 -52
- Misses 75 83 +8 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #101 will improve performances by 79.69%Comparing Summary
Benchmarks breakdown
|
WalkthroughThe pull request introduces changes to the Changes
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
🔇 Additional comments (3)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
cpp-linter/src/clang_tools/clang_format.rs (3)
156-156: Add test coverage for deserialization error handlingThe error handling for non-UTF-8 encoded XML output is crucial, but static analysis indicates this line isn't covered by tests. Consider adding tests that provide invalid or non-UTF-8 encoded XML data to ensure the error handling works as expected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 156-156: cpp-linter/src/clang_tools/clang_format.rs#L156
Added line #L156 was not covered by tests
165-167: Ensure file read errors are testedThe added error context when reading the original file content improves debuggability. However, static analysis reports that these lines lack test coverage. Add test cases simulating scenarios where reading the file might fail, such as missing files or permission issues, to validate the error handling logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 165-167: cpp-linter/src/clang_tools/clang_format.rs#L165-L167
Added lines #L165 - L167 were not covered by tests
205-222: Update tests to cover new deserialization logicThe
parse_xmltest has been updated to usequick_xml, but ensure it adequately covers various scenarios, including edge cases and error handling. Consider adding tests for empty inputs, malformed XML, and unexpected structures to verify robustness.cpp-linter/src/common_fs/mod.rs (1)
315-316: Enhance test coverage for boundary conditionsIn
translate_byte_offsettest, consider adding cases whereoffsetis zero, at the end of the file, or beyond the file length to ensureget_line_cols_from_offsethandles these scenarios correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
cpp-linter/Cargo.toml(1 hunks)cpp-linter/src/clang_tools/clang_format.rs(5 hunks)cpp-linter/src/common_fs/mod.rs(3 hunks)cpp-linter/src/rest_api/github/mod.rs(0 hunks)cpp-linter/src/rest_api/github/specific_api.rs(1 hunks)
💤 Files with no reviewable changes (1)
- cpp-linter/src/rest_api/github/mod.rs
✅ Files skipped from review due to trivial changes (1)
- cpp-linter/src/rest_api/github/specific_api.rs
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cpp-linter/src/clang_tools/clang_format.rs
[warning] 156-156: cpp-linter/src/clang_tools/clang_format.rs#L156
Added line #L156 was not covered by tests
[warning] 165-167: cpp-linter/src/clang_tools/clang_format.rs#L165-L167
Added lines #L165 - L167 were not covered by tests
🔇 Additional comments (4)
cpp-linter/src/clang_tools/clang_format.rs (3)
5-5: Import offsmodule is appropriateAdding
fsto the imports is necessary for file operations used later in the code.
41-41: Addition ofCopytrait is acceptableDeriving the
Copytrait forReplacementallows for simpler value semantics since the struct is composed of types that implementCopy.
24-24:⚠️ Potential issueCorrect the
serdeattribute syntax for field renamingThe attribute
#[serde(rename(deserialize = "replacement"))]may not correctly specify the field rename during deserialization. The correct syntax inserdefor renaming a field during deserialization is#[serde(rename = "replacement")]or, if you need to differentiate between serialization and deserialization names, use#[serde(rename(serialize = "name1", deserialize = "name2"))].Apply this diff to fix the attribute:
- #[serde(rename(deserialize = "replacement"))] + #[serde(rename = "replacement")]Likely invalid or redundant comment.
cpp-linter/Cargo.toml (1)
28-28: Dependency update toquick-xmlis appropriateAdding
quick-xmlwith theserializefeature aligns with the shift fromserde-xml-rsand provides efficient XML parsing capabilities.
Also ensure given `offset` is not out of bounds when calculating line number from byte offset.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp-linter/src/clang_tools/clang_format.rs (1)
41-52: Consider adding documentation for the removed fields.While removing unused fields is good, it would be helpful to document why
lengthandvaluefields were removed to maintain historical context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cpp-linter/src/clang_tools/clang_format.rs(7 hunks)cpp-linter/src/clang_tools/clang_tidy.rs(1 hunks)cpp-linter/src/clang_tools/mod.rs(5 hunks)cpp-linter/src/common_fs/file_filter.rs(1 hunks)cpp-linter/src/common_fs/mod.rs(3 hunks)cpp-linter/src/rest_api/github/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp-linter/src/rest_api/github/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: aarch64-pc-windows-msvc
- GitHub Check: x86_64-pc-windows-msvc
- GitHub Check: x86_64-apple-darwin
- GitHub Check: aarch64-apple-darwin
- GitHub Check: s390x-unknown-linux-gnu
- GitHub Check: powerpc64le-unknown-linux-gnu
- GitHub Check: powerpc64-unknown-linux-gnu
- GitHub Check: powerpc-unknown-linux-gnu
- GitHub Check: armv7-unknown-linux-gnueabihf
- GitHub Check: arm-unknown-linux-gnueabihf
- GitHub Check: arm-unknown-linux-gnueabi
- GitHub Check: stable - x86_64-unknown-linux-gnu - node@20
- GitHub Check: test (ubuntu-latest)
- GitHub Check: stable - x86_64-pc-windows-msvc - node@20
- GitHub Check: aarch64-unknown-linux-musl
- GitHub Check: aarch64-unknown-linux-gnu
- GitHub Check: benchmark
- GitHub Check: test (windows-latest)
- GitHub Check: stable - x86_64-apple-darwin - node@20
🔇 Additional comments (10)
cpp-linter/src/common_fs/file_filter.rs (1)
114-117: LGTM! Improved log message clarity.The updated debug message format is clearer and more direct, making it easier to understand the ignore status of files during debugging.
cpp-linter/src/clang_tools/clang_tidy.rs (1)
279-289: LGTM! Clean refactoring of the original content handling.The code has been simplified by using a more direct conditional assignment pattern, improving readability while maintaining the same functionality.
cpp-linter/src/clang_tools/mod.rs (3)
261-271: LGTM! Improved type safety in loop indexing.The change from
u8tousizefor array indexing aligns with Rust's best practices for array access.
Line range hint
371-384: LGTM! Simplified boolean to index conversion.The code now uses a more idiomatic Rust approach by casting the boolean comparison directly to
usize.
447-449: LGTM! Optimized tool total handling.The code now uses a more concise approach to update tool totals with
unwrap_or_default.cpp-linter/src/clang_tools/clang_format.rs (3)
Line range hint
21-28: LGTM! Clean struct definition with appropriate attributes.The
FormatAdvicestruct is well-defined with proper serialization attributes for quick-xml.
146-158: LGTM! Improved XML parsing with proper error handling.The code now properly handles XML parsing with appropriate error context and empty XML cases.
192-197: LGTM! Added test for blank XML handling.Good addition of a test case to verify behavior with blank XML input.
cpp-linter/src/common_fs/mod.rs (2)
230-234: LGTM! Improved offset handling with bounds checking.The function has been simplified to only return line count and includes proper bounds checking with
min(). The removal of column calculation is appropriate since it was unused in the codebase.
314-316: LGTM! Updated test case for new function signature.The test has been properly updated to work with the new function signature and simplified return value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cpp-linter/src/common_fs/mod.rs (2)
222-229: Clean up documentation to remove column-related references.The documentation still mentions "column number" and "word boundary" which are no longer relevant since the function now only computes line numbers.
/// Gets the line number for a given `offset` (of bytes) from given the /// byte array `contents`. /// /// This computes the line number from a buffer of bytes. -/// In non-UTF-8 encoded files, this does not guarantee that a word -/// boundary exists at the returned column number. However, the `offset` given to this +/// The `offset` given to this /// function is expected to originate from diagnostic information provided by /// clang-format.
313-315: Consider enhancing test coverage.While the current test is good, consider adding tests for:
- Empty file (offset 0)
- Offset at end of file
- Offset beyond file length
- File with no newlines
- File with consecutive newlines
Also, consider using a test fixture or constant for the test file path to make tests more maintainable.
Example test cases:
#[test] fn test_line_count_edge_cases() { // Empty content assert_eq!(get_line_count_from_offset(&[], 0), 1); // No newlines assert_eq!(get_line_count_from_offset(b"abc", 3), 1); // Consecutive newlines assert_eq!(get_line_count_from_offset(b"a\n\nb", 3), 3); // Offset beyond content length assert_eq!(get_line_count_from_offset(b"a\nb\n", 10), 3); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp-linter/src/common_fs/mod.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: aarch64-pc-windows-msvc
- GitHub Check: x86_64-apple-darwin
- GitHub Check: aarch64-apple-darwin
- GitHub Check: s390x-unknown-linux-gnu
- GitHub Check: powerpc64le-unknown-linux-gnu
- GitHub Check: powerpc64-unknown-linux-gnu
- GitHub Check: powerpc-unknown-linux-gnu
- GitHub Check: armv7-unknown-linux-gnueabihf
- GitHub Check: arm-unknown-linux-gnueabihf
- GitHub Check: arm-unknown-linux-gnueabi
- GitHub Check: stable - x86_64-unknown-linux-gnu - node@20
- GitHub Check: x86_64-unknown-linux-gnu
- GitHub Check: stable - x86_64-pc-windows-msvc - node@20
- GitHub Check: test (ubuntu-latest)
- GitHub Check: aarch64-unknown-linux-musl
- GitHub Check: aarch64-unknown-linux-gnu
- GitHub Check: stable - x86_64-apple-darwin - node@20
- GitHub Check: test (windows-latest)
- GitHub Check: benchmark
🔇 Additional comments (1)
cpp-linter/src/common_fs/mod.rs (1)
230-233: LGTM! Clean and safe implementation.The implementation correctly handles bounds checking and efficiently counts lines. The use of
splitwith newline detection is an appropriate approach.
cols,length, andvaluefields were never used and are now ignored when parsing XMLand columnsSummary by CodeRabbit
Dependency Changes
quick-xmllibrary with serialization supportserde-xml-rsdependencyCode Improvements
Performance
Logging Enhancements