Clippy clean and coverage #14
Conversation
Reviewer's GuideThis PR focuses on improving code clarity and robustness by adding comprehensive test coverage, simplifying impl lifetimes, removing redundant clones, reformatting documentation and error handling, and updating project TODOs and docs. Class diagram for PullParser and StreamParser test coverage and API changesclassDiagram
class PullParser {
+with_config(input)
+with_config_and_buffer(input, scratch_buffer)
+next_event()
}
class StreamParser {
+new(reader, buffer)
+next_event()
}
PullParser <|-- BitStackStruct
PullParser <|-- ArrayBitStack
StreamParser <|-- SliceReader
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@kaidokert has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 27 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 (2)
WalkthroughThis set of changes primarily updates lifetime annotations in several Rust modules, shifting from explicit named lifetimes to anonymous lifetimes for trait implementations and struct methods. It also introduces new and expanded test coverage for parsing, error handling, and ergonomic APIs, refines documentation and configuration, and renames the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PullParser
participant Config
participant Buffer
participant BitStackConfig
User->>PullParser: create with_config(config)
PullParser->>Config: use config
PullParser->>User: parse JSON (borrowed strings)
User->>PullParser: create with_config_and_buffer(config, buffer)
PullParser->>Config: use config
PullParser->>Buffer: use buffer for escapes
PullParser->>User: parse JSON (with escape handling)
User->>PullParser: create with alternative BitStackConfig
PullParser->>BitStackConfig: use for deep nesting
PullParser->>User: parse deeply nested JSON
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @kaidokert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily focuses on improving the overall code quality, maintainability, and test coverage of the picojson crate. It addresses various static analysis warnings, refines internal logic, and significantly expands the test suite to ensure robustness and correctness across different parsing scenarios and API usages.
Highlights
- Code Quality & Clippy Cleanups: I've applied various Clippy suggestions to improve code quality and readability. This includes simplifying explicit lifetime annotations to inferred lifetimes (
'_) acrossDirectBuffer,JsonNumber,String,SliceInputBuffer, andStreamParserimplementations. Additionally, I've removed unnecessary string allocations in error messages and streamlined conditional checks for conciseness. - Enhanced Test Coverage: I've significantly expanded the test suite to improve code coverage. This includes new tests for
PullParserconstructors and deep nesting scenarios, comprehensive tests forContentRangeutility functions andParserErrorHandlerinshared.rs, and dedicated tests forStreamParser's escape buffer handling. A new integration test file (json_number_ergonomics_test.rs) was added to thoroughly test theJsonNumber'sDisplay,Deref,AsRef,parse,is_integer,is_float,as_f64, andas_intmethods. - Minor Performance & Code Refinements: I've made small but impactful changes like removing redundant
.clone()calls in theArrayBitStackimplementation, which can lead to minor performance improvements forCopytypes. Test values injson_number.rsandstream_parser.rswere also updated for consistency. - Project TODO List Update: The
TODO.mdfile has been updated to reflect current development priorities, removing completed or deprioritized items and adding new goals such as unifyingPullParserandStreamParsercode and re-evaluating iterator implementations.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kaidokert - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request focuses on improving code quality by addressing Clippy warnings, such as removing redundant clones and using more idiomatic constructs. It also significantly expands test coverage for various modules, including new integration tests for JsonNumber ergonomics. The changes are generally excellent and improve the codebase. I've found one issue in a test case where an incorrect value is used for an assertion.
![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 comments (1)
picojson/src/json_number.rs (1)
177-177: Fix unused variable warning.The pipeline indicates that variable
sis unused. This should be addressed to clean up compiler warnings.Based on the pipeline failure, prefix the variable with an underscore if it's intentionally unused:
-pub(super) fn parse_float(s: &str) -> Result<NumberResult, ParseError> { +pub(super) fn parse_float(_s: &str) -> Result<NumberResult, ParseError> {
🧹 Nitpick comments (1)
picojson/tests/json_number_ergonomics_test.rs (1)
144-144: Consider configuration-specific JSON data.As an alternative to conditional compilation around the parsing logic, you could use different JSON data based on feature configuration to avoid floats entirely when
float-erroris enabled.- let json = r#"[42, 3.25]"#; + #[cfg(feature = "float-error")] + let json = r#"[42, 123]"#; // Use integer instead of float for float-error config + #[cfg(not(feature = "float-error"))] + let json = r#"[42, 3.25]"#;Then adjust the test assertions accordingly for the integer case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md(1 hunks)TODO.md(1 hunks)picojson/src/direct_buffer.rs(1 hunks)picojson/src/json_number.rs(4 hunks)picojson/src/json_string.rs(3 hunks)picojson/src/pull_parser.rs(5 hunks)picojson/src/shared.rs(1 hunks)picojson/src/slice_input_buffer.rs(2 hunks)picojson/src/stream_parser.rs(5 hunks)picojson/src/tokenizer/bitstack/mod.rs(3 hunks)picojson/tests/json_number_ergonomics_test.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
TODO.md (2)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
picojson/src/slice_input_buffer.rs (2)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
picojson/src/direct_buffer.rs (2)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
picojson/src/tokenizer/bitstack/mod.rs (2)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
picojson/src/shared.rs (1)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
picojson/src/json_number.rs (2)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
picojson/src/json_string.rs (2)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
picojson/src/pull_parser.rs (4)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: stax/src/lib.rs:19-21
Timestamp: 2025-06-28T18:14:22.845Z
Learning: In Rust crate organization, functions can be imported into lib.rs via `use` statements not for direct usage within lib.rs itself, but to make them available to other modules within the same crate via the `crate::` path. This is a valid pattern and such imports should not be flagged as unused even if lib.rs doesn't directly call them.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: stax/src/flex_parser.rs:0-0
Timestamp: 2025-06-28T18:12:29.968Z
Learning: In the stax JSON parser codebase, EscapeSequence event handlers exist in flex_parser.rs not because they're needed by that parser variant, but to avoid catch-all patterns in match statements. The flex parser doesn't need to process EscapeSequence events, but the other parser variant (direct parser) does need them.
picojson/tests/json_number_ergonomics_test.rs (2)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
picojson/src/stream_parser.rs (3)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: stax/src/flex_parser.rs:0-0
Timestamp: 2025-06-28T18:12:29.968Z
Learning: In the stax JSON parser codebase, EscapeSequence event handlers exist in flex_parser.rs not because they're needed by that parser variant, but to avoid catch-all patterns in match statements. The flex parser doesn't need to process EscapeSequence events, but the other parser variant (direct parser) does need them.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
🪛 GitHub Actions: Build and test
picojson/src/shared.rs
[warning] 330-330: Rust warning: calls to std::str::from_utf8 with an invalid literal always return an error.
picojson/src/json_number.rs
[warning] 177-177: Rust warning: unused variable s. Suggestion: prefix with underscore if intentional.
picojson/tests/json_number_ergonomics_test.rs
[error] 37-37: Test failure in test_json_number_display_trait: called Result::unwrap() on an Err value: FloatNotAllowed.
[error] 159-159: Test failure in test_json_number_type_checking_methods: called Result::unwrap() on an Err value: FloatNotAllowed.
picojson/src/stream_parser.rs
[error] 1248-1248: Test failure in stream_parser::tests::test_direct_parser_no_float_configuration: assertion failed: (f - 3.25).abs() < 0.01
🪛 GitHub Actions: Coverage
picojson/src/shared.rs
[warning] 328-330: Warning: calls to std::str::from_utf8 with an invalid literal always return an error. #[warn(invalid_from_utf8)] on by default.
picojson/src/stream_parser.rs
[error] 1248-1248: Test failure in stream_parser::tests::test_direct_parser_no_float_configuration: assertion failed: (f - 3.25).abs() < 0.01
🔇 Additional comments (22)
README.md (1)
98-98: LGTM! Proper file formatting.Adding the missing newline at the end of the file is a good practice and likely resolves a linting issue.
TODO.md (1)
7-7: Good additions to the TODO list.These items reflect logical next steps for the codebase:
- Unifying PullParser and StreamParser code would reduce duplication
- Bringing back an Iterator implementation would improve ergonomics
Both align well with the ongoing improvements in the parsing modules.
Also applies to: 15-15
picojson/src/tokenizer/bitstack/mod.rs (1)
138-139: Excellent optimization! Removing unnecessary clone() calls.Since
T: Copyis required in the trait bounds (line 125), these clone() calls are redundant. Direct field access is more efficient for Copy types, eliminating unnecessary method calls.This change improves performance while maintaining identical semantics.
Also applies to: 147-147, 156-157, 166-166
picojson/src/slice_input_buffer.rs (1)
25-25: Good ergonomic improvement with anonymous lifetimes.Replacing explicit lifetime parameters with
'_simplifies the code without changing functionality. The compiler can infer the lifetimes correctly from context, reducing visual noise.Also applies to: 57-57
picojson/src/direct_buffer.rs (1)
462-462: Consistent lifetime simplification across the codebase.Using anonymous lifetime
'_maintains consistency with similar changes inslice_input_buffer.rs. This reduces cognitive overhead while preserving the same trait implementation behavior.picojson/src/json_string.rs (3)
16-16: LGTM: Lifetime annotation simplification.The change from explicit lifetime parameters to anonymous lifetimes improves code ergonomics while maintaining the same functionality.
26-26: LGTM: Consistent lifetime simplification.Good consistency with the inherent impl block - using anonymous lifetimes for the
AsRef<str>trait implementation.
43-43: LGTM: Completes the lifetime annotation cleanup.The
Displaytrait implementation now consistently uses anonymous lifetimes, completing the ergonomic improvements for this module.picojson/src/json_number.rs (4)
53-53: LGTM: Lifetime annotation simplification.Consistent with the pattern across the codebase - replacing explicit lifetime parameters with anonymous lifetimes for better ergonomics.
118-118: LGTM: Consistent trait implementation cleanup.The
AsRef<str>trait implementation now uses anonymous lifetimes, maintaining consistency with the codebase patterns.
132-132: LGTM: Completes lifetime simplification.The
Displaytrait implementation consistently uses anonymous lifetimes, completing the ergonomic improvements.
285-290: LGTM: Test data update.The float test values were changed from "3.14159"/3.14159 to "3.25"/3.25. This appears to be a minor test data change that doesn't affect the test logic.
picojson/src/shared.rs (1)
248-343: Excellent test coverage additions.The new test module provides comprehensive coverage for utility functions including:
- Unicode escape boundary calculations
- String content boundary calculations
- Error constructor functions
- UTF-8 validation error handling
These tests strengthen the foundation for error handling and parsing utilities used throughout the crate.
picojson/src/pull_parser.rs (5)
87-88: LGTM: Documentation formatting improvement.Better line wrapping improves readability of the documentation comment.
141-142: LGTM: Consistent documentation formatting.Matches the formatting pattern established in the previous documentation comment.
442-442: LGTM: String literal optimization.Good practice to use string slice literals instead of owned strings for static error messages.
452-452: LGTM: Test imports for new test functions.The additional imports (
ArrayBitStack,BitStackStruct) are needed for the new test functions that follow.
819-883: Excellent test coverage expansion.The three new test functions provide valuable coverage:
test_with_config_constructors- Tests alternative constructor with custom BitStack configtest_with_config_and_buffer_constructors- Tests constructor with both config and scratch buffertest_alternative_config_deep_nesting- Tests deep nesting handling with custom BitStackThese tests ensure the alternative construction paths and configurations work correctly, improving the robustness of the parser API.
picojson/src/stream_parser.rs (3)
121-121: LGTM: Lifetime simplification improves readability.The change from explicit lifetime
'bto anonymous lifetime'_aligns with the broader crate-wide effort to simplify lifetime annotations while maintaining the same functionality.
160-160: LGTM: Streamlined error handling.The use of
.is_err()instead of explicitif let Err(_)pattern matching is more idiomatic and improves readability when the error value is not needed.Also applies to: 186-186
1485-1542: LGTM: Well-designed tests improve coverage.The three new test functions effectively cover previously untested code paths:
test_escape_buffer_functionsexercises escape processing through the public APItest_slice_reader_constructorvalidates the constructor behaviortest_complex_escape_sequencestests comprehensive escape sequence handlingThese tests follow good practices and strengthen the robustness of the parsing functionality.
picojson/tests/json_number_ergonomics_test.rs (1)
4-46: LGTM: Comprehensive test coverage for JsonNumber ergonomics.The test suite effectively validates all the ergonomic APIs of
JsonNumber:
- Display trait formatting
- Deref trait for string access
- AsRef trait for generic functions
- Parse method for type conversions
- Type checking methods (is_integer, is_float)
- Conversion methods (as_f64, as_int)
The tests are well-structured with clear assertions and proper error handling. The
as_f64test already has appropriate feature guards, which is good practice.Also applies to: 48-113, 115-196, 198-228
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.pre-commit-config.yaml (2)
25-90: Execution time: 8 serialcargo checks may hurt DXRunning eight full compilations on every commit can slow the feedback loop noticeably, especially on CI or slower laptops. Two ideas:
- Gate the exhaustive matrix behind the (much rarer)
pre-pushstage:stages: [push]- Or keep it in CI and let the pre-commit hook only run the fast “default” build.
Decide whether developer ergonomics or commit-time guarantees are the higher priority here.
30-90: Nit: reuse common command prefix to reduce duplicationEvery hook repeats the same command with only the feature list changing. Switching to an executable script (e.g.
scripts/feature_check.sh "$@") or using an env-substitutedentry:shortens the file and avoids copy-paste drift.Not urgent, but improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.pre-commit-config.yaml(1 hunks)picojson/examples/no_float_demo.rs(1 hunks)picojson/src/json_number.rs(6 hunks)picojson/src/lib.rs(1 hunks)picojson/src/shared.rs(1 hunks)picojson/src/stream_parser.rs(6 hunks)picojson/src/ujson/bitstack/mod.rs(4 hunks)picojson/src/ujson/tokenizer/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- picojson/src/ujson/bitstack/mod.rs
- picojson/src/ujson/tokenizer/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- picojson/src/shared.rs
- picojson/src/stream_parser.rs
- picojson/src/json_number.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
picojson/examples/no_float_demo.rs (2)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: stax/src/flex_parser.rs:0-0
Timestamp: 2025-06-28T18:12:29.968Z
Learning: In the stax JSON parser codebase, EscapeSequence event handlers exist in flex_parser.rs not because they're needed by that parser variant, but to avoid catch-all patterns in match statements. The flex parser doesn't need to process EscapeSequence events, but the other parser variant (direct parser) does need them.
picojson/src/lib.rs (2)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
🧬 Code Graph Analysis (1)
picojson/examples/no_float_demo.rs (1)
picojson/src/json_number.rs (1)
parsed(51-56)
🔇 Additional comments (4)
picojson/src/lib.rs (2)
56-56: LGTM: Clean module rename from tokenizer to ujsonThe module declaration change is consistent with the overall refactoring mentioned in the PR objectives.
57-60: LGTM: Consistent update of public re-exportsAll public re-exports have been correctly updated from
tokenizer::toujson::paths, maintaining the same public API surface while completing the module rename. This aligns with the retrieved learning about removing the transitionary alias.picojson/examples/no_float_demo.rs (1)
114-121: LGTM! Excellent defensive programming.The catch-all arm is a smart addition that prevents match exhaustiveness issues and provides valuable debugging information. Given that
NumberResultnow includes all variants regardless of feature flags (as mentioned in the PR summary), this ensures any unexpected variants are properly reported rather than causing compilation failures..pre-commit-config.yaml (1)
42-69: Missingint32 + floatvariants for all float modesYou cover
float,float-skip,float-error, andfloat-truncateforint32, but forint64you only test the*-skip|error|truncatepermutations – the plainint64 + floatcombination is absent.
Add the symmetrical check for completeness.- id: feature-matrix-int64-float name: Feature Matrix Check - int64 + float entry: cargo check --manifest-path=picojson/Cargo.toml --no-default-features --features "int64,float"
Summary by Sourcery
Clean up codebase for Clippy warnings, refactor impl lifetimes and remove redundant code, and expand test coverage across shared, pull_parser, stream_parser, and json_number modules.
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
Style
New Features
Bug Fixes
Refactor
Chores