add defmt feature - #92
Conversation
Adds the necessary `derive`s for printing picojson's data types using defmt; gated behind a feature `defmt`, as is customary in the rust embedded/defmt ecosystem.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Added the derives for |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="picojson/src/ujson/tokenizer/mod.rs" line_range="312-313" />
<code_context>
}
}
+#[cfg(feature = "defmt")]
+impl defmt::Format for Error {
+ fn format(&self, f: defmt::Formatter) {
+ defmt::write!(
</code_context>
<issue_to_address>
**suggestion:** Consider delegating to the existing `Debug` impl via `defmt::Debug2Format` to avoid format duplication.
Right now `Error` has two separate formatting implementations that must be kept in sync. To avoid drift, implement `defmt::Format` in terms of the existing `Debug` impl, e.g. call `defmt::Debug2Format(self)` inside `defmt::write!` so any future `Debug` changes are automatically reflected in defmt output.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
picojson/src/ujson/tokenizer/mod.rs (1)
220-224: Optional: Consider addingdefmt::FormattoTokenizer.The
Tokenizerstruct doesn't have adefmt::Formatderive, though all its fields now support formatting. While parser state formatting might be verbose and infrequently needed in embedded contexts, adding the derive could be helpful for debugging parser behavior.📝 Optional addition
/// Low-level JSON tokenizer that tracks parsing state and line/column positions. +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct Tokenizer<T: BitBucket = u32, D = u8> { state: State, context: ParseContext<T, D>, position: Position, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@picojson/src/ujson/tokenizer/mod.rs` around lines 220 - 224, Add a defmt::Format derive to the Tokenizer struct and constrain its generics so the fields support formatting: add #[derive(defmt::Format)] above pub struct Tokenizer and change the generic bounds to something like Tokenizer<T: BitBucket + defmt::Format, D: defmt::Format> (ensuring State, ParseContext<T,D>, and Position implement defmt::Format as well via their own derives or bounds); this enables defmt logging of the Tokenizer (fields: state, context, position) without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@picojson/src/ujson/tokenizer/mod.rs`:
- Around line 220-224: Add a defmt::Format derive to the Tokenizer struct and
constrain its generics so the fields support formatting: add
#[derive(defmt::Format)] above pub struct Tokenizer and change the generic
bounds to something like Tokenizer<T: BitBucket + defmt::Format, D:
defmt::Format> (ensuring State, ParseContext<T,D>, and Position implement
defmt::Format as well via their own derives or bounds); this enables defmt
logging of the Tokenizer (fields: state, context, position) without changing
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bab9aaf-7b6b-44da-bfec-f8b9165c3691
📒 Files selected for processing (17)
.github/workflows/build.yamlpicojson/Cargo.tomlpicojson/src/chunk_reader.rspicojson/src/escape_processor.rspicojson/src/event_processor.rspicojson/src/int_parser.rspicojson/src/json_number.rspicojson/src/json_string.rspicojson/src/parse_error.rspicojson/src/push_parser.rspicojson/src/shared.rspicojson/src/slice_input_buffer.rspicojson/src/stream_buffer.rspicojson/src/ujson/bitstack/mod.rspicojson/src/ujson/tokenizer/mod.rspicojson/tests/push_parser_stress_test.rspicojson/tests/selective_extraction.rs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for the defmt logging framework by introducing a defmt feature and deriving defmt::Format for numerous internal and public types. A critical issue was identified in Cargo.toml where the specified defmt version (1.0.1) is non-existent on crates.io, which would cause dependency resolution to fail.
|
Thanks for putting this up and sorry for late review |
That was my first thought looking at it actually - you may not need most of them, unless we add specific unit tests specifically verifying defmt that would require them. The key things that likely would need defmt are all public-exported structs and enums ( list here , about ~13 of them ). Of course these may transitively require some inner types to be defmt as well. I'm also gonna guess tagging Would you mind try doing those other ~10 only ? |
Removed the annotations on inner types that don't propagate to public interfaces.
|
Merged this with the trimmed state - will make a new release, PTAL and let me know if this works |
2 participants
Adds the necessary
derives for printing picojson's data types using defmt; gated behind a featuredefmt, as is customary in the rust embedded/defmt ecosystem.Closes #37
Summary by Sourcery
Add optional defmt support across core picojson types to enable structured logging in embedded environments.
New Features:
defmtfeature flag to enable formatted logging of picojson types via the defmt ecosystem.Enhancements:
defmt::Formatfor key public and internal data structures, including events, numbers, tokenizer states, and error enums when thedefmtfeature is enabled.defmt::Formatimplementation for the tokenizerErrortype to include detailed position and character information in logs.Build:
defmtdependency and wire it to the newdefmtfeature inCargo.toml.CI:
defmtfeature enabled.Summary by CodeRabbit
New Features
defmtfeature flag enabling enhanced debug formatting support for JSON parsing types and structures.Chores
Tests