feat(third_party): add support for serde_json::Value#374
Merged
Conversation
|
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)
📝 WalkthroughWalkthroughAdds optional Changes
Sequence DiagramsequenceDiagram
participant Builder as Config Builder
participant TomlSource as TOML Source
participant JsonValue as serde_json::Value
participant Wrapper as Wrapper
participant ForeignType as ForeignType
Builder->>TomlSource: override_with(TOML 1)
TomlSource-->>JsonValue: produce Value
Builder->>TomlSource: override_with(TOML 2)
TomlSource-->>JsonValue: produce/merge Value
JsonValue->>JsonValue: merge (primitives/arrays/objects)
JsonValue->>Wrapper: wrap for field
Wrapper->>ForeignType: TryFrom conversion (serde_json::from_value)
ForeignType-->>Builder: constructed field
Builder->>Builder: try_build() -> Config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@confik/CHANGELOG.md`:
- Around line 16-25: Fix the example in the TryFrom implementation and the
Config struct: change the call inside fn try_from(wrapper: Wrapper) from
Self::from_value(wrapper.0) to serde_json::from_value(wrapper.0) so it calls the
correct serde_json function, and correct the field name in the
#[derive(Configuration)] struct Config from forgein_type to foreign_type (the
attribute #[confik(try_from = Wrapper)] and the struct name Config remain the
same).
In `@confik/examples/foreign_type_via_value.rs`:
- Around line 1-43: Add a [[example]] entry to confik/Cargo.toml specifying
required-features so the example foreign_type_via_value.rs can compile with toml
and serde_json; specifically, add an example block with name
"foreign_type_via_value" and required-features ["toml","serde_json"] so that the
example that uses TomlSource and serde_json (see types Wrapper, ForeignType,
Config and their use in main) is built with the needed features.
In `@confik/src/third_party.rs`:
- Around line 285-301: The merge implementation currently keeps earlier
primitive values by returning the left-hand operand; update the primitive match
arm in the merge method so that when merging primitives (patterns Self::Null,
Self::Bool(_), Self::Number(_), Self::String(_)) it returns the right-hand
operand (`other`) instead of `self`, i.e. flip the pattern to match the
primitive on the right side so later sources (the `other` passed from
reduce/override_with) take precedence; leave the Array and Object arms (extend
semantics) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1393b2e-e169-45bc-bacc-492e93014b92
📒 Files selected for processing (4)
confik/CHANGELOG.mdconfik/Cargo.tomlconfik/examples/foreign_type_via_value.rsconfik/src/third_party.rs
tenuous-guidance
force-pushed
the
easy-foreign-types
branch
from
a9419ac to
e19e54b
Compare
This gives a very simple way of adding support for foreign types, as per the provided example.
tenuous-guidance
force-pushed
the
easy-foreign-types
branch
from
e19e54b to
f50081d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
confik/tests/third_party.rs (1)
154-159: Add one overlapping-key merge assertion.This only locks in the disjoint-key case.
confik/src/third_party.rs:284-301also defines what happens when both objects contain the same key, and that precedence rule is the behavior most likely to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@confik/tests/third_party.rs` around lines 154 - 159, The test merge_objects_combine_disjoint_keys only covers disjoint keys; add an overlapping-key assertion inside the same test (merge_objects_combine_disjoint_keys) that calls the same merge method (JSON::merge) with two objects sharing a key and assert the expected precedence (the right-hand object's value should override the left-hand), e.g. assert that merging json!({"a": 1}) with json!({"a": 2}) yields json!({"a": 2}).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@confik/tests/third_party.rs`:
- Around line 130-133: The test module named `mod serde_json { ... }` shadows
the external crate, so change the module name (e.g., `mod serde_json_tests`) or
import the external crate with an absolute path (replace `use serde_json::{json,
Value};` with `use ::serde_json::{json, Value};`) so `json` and `Value` resolve
to the external crate; update any other `serde_json` references inside the
module (including the `mod` declaration and imports) to match the rename or
absolute import.
---
Nitpick comments:
In `@confik/tests/third_party.rs`:
- Around line 154-159: The test merge_objects_combine_disjoint_keys only covers
disjoint keys; add an overlapping-key assertion inside the same test
(merge_objects_combine_disjoint_keys) that calls the same merge method
(JSON::merge) with two objects sharing a key and assert the expected precedence
(the right-hand object's value should override the left-hand), e.g. assert that
merging json!({"a": 1}) with json!({"a": 2}) yields json!({"a": 2}).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e026e508-8fb8-42f5-875c-b82cfbcd33e8
📒 Files selected for processing (5)
confik/CHANGELOG.mdconfik/Cargo.tomlconfik/examples/foreign_type_via_value.rsconfik/src/third_party.rsconfik/tests/third_party.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- confik/src/third_party.rs
- confik/examples/foreign_type_via_value.rs
robjtede
approved these changes
Mar 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This gives a very simple way of adding support for foreign types, as per the provided example.
Summary by CodeRabbit
New Features
Documentation
Tests