Closed
Conversation
tenuous-guidance
force-pushed
the
json5
branch
from
dfa6659 to
0bc18a8
Compare
tenuous-guidance
force-pushed
the
json5
branch
from
0bc18a8 to
732d7d2
Compare
Contributor
Author
$ cargo update -p=toml_edit --precise=0.21.0
error: There are multiple `toml_edit` packages in your project, and the specification `toml_edit` is ambiguous.
Please re-run this command with one of the following specifications:
toml_edit@0.21.1
toml_edit@0.22.20
$ cargo tree -i toml_edit
toml_edit v0.21.1
└── toml v0.8.8
├── confik v0.11.7
│ [dev-dependencies]
│ └── confik-macros v0.11.1 (proc-macro)
│ └── confik v0.11.7 (*)
└── trybuild v1.0.99
[dev-dependencies]
└── confik-macros v0.11.1 (proc-macro) (*)
[dev-dependencies]
└── confik-macros v0.11.1 (proc-macro) (*)
$ cargo tree -i toml_edit@0.22.20
error: package ID specification `toml_edit@0.22.20` did not match any packages
Did you mean one of these?
toml_edit@0.21.1
$ cargo update -p=toml_edit@0.22.20 --precise=0.21.0
Updating crates.io index
error: failed to select a version for the requirement `toml_edit = "^0.22.20"`
candidate versions found which didn't match: 0.21.0
location searched: crates.io index
required by package `proc-macro-crate v3.2.0`
... which satisfies dependency `proc-macro-crate = "^3"` (locked to 3.2.0) of package `borsh-derive v1.5.1`
... which satisfies dependency `borsh-derive = "~1.5.1"` (locked to 1.5.1) of package `borsh v1.5.1`
... which satisfies dependency `borsh = "^1.1.1"` (locked to 1.5.1) of package `rust_decimal v1.36.0`
... which satisfies dependency `rust_decimal = "^1"` (locked to 1.36.0) of package `confik v0.11.7 (/home/user/git_repo/3pp/confik/confik)`
... which satisfies dependency `confik = "^0.11"` (locked to 0.11.7) of package `confik-macros v0.11.1 (/home/user/git_repo/3pp/confik/confik-macros)`I think we're hitting a bug in How would you like to proceed @robjtede? |
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request introduces JSON5 support as an optional feature to the confik configuration library. It adds a new Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant J5S as Json5Source
participant J5 as json5 Parser
participant Config as ConfigurationBuilder
App->>J5S: new(json5_contents)
activate J5S
J5S->>J5S: store contents & allow_secrets flag
deactivate J5S
App->>J5S: provide::<Config>()
activate J5S
J5S->>J5: from_str(contents)
activate J5
J5-->>J5S: parsed JSON5 data
deactivate J5
J5S->>Config: deserialize parsed data
activate Config
Config-->>J5S: Config<T>
deactivate Config
J5S-->>App: Result<T, Error>
deactivate J5S
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/src/sources/json5_source.rs`:
- Around line 5-14: The public doc comments for the Json5Source type and its
constructor incorrectly say "JSON" — update them to say "JSON5" (e.g., change "A
[`Source`] containing raw JSON data." and "Creates a [`Source`] containing raw
JSON data." to mention "raw JSON5 data" and optionally note that this accepts
JSON5 syntax/extensions). Edit the doc comments on the Json5Source struct and
impl/new function to reference JSON5 and JSON5-specific parsing behavior so
docs.rs and IDE hovers show the correct format.
- Around line 28-35: The impl currently omits the trait generic and introduces a
local T in provide; change the impl to implement the generic trait by declaring
impl<'a, T: ConfigurationBuilder> Source<T> for Json5Source<'a>, keep
allows_secrets(&self) as-is, and update provide to match the trait signature fn
provide(&self) -> Result<T, Box<dyn Error + Sync + Send>> and return
Ok(json5::from_str::<T>(&self.contents)?); this ensures Json5Source implements
Source<T> for the appropriate T.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60dc5ca7-4533-4583-a97a-20b51eb192cb
📒 Files selected for processing (5)
confik/CHANGELOG.mdconfik/Cargo.tomlconfik/src/lib.rsconfik/src/sources/json5_source.rsconfik/src/sources/mod.rs
Comment on lines
+5
to
+14
| /// A [`Source`] containing raw JSON data. | ||
| #[derive(Clone)] | ||
| pub struct Json5Source<'a> { | ||
| contents: Cow<'a, str>, | ||
| allow_secrets: bool, | ||
| } | ||
|
|
||
| impl<'a> Json5Source<'a> { | ||
| /// Creates a [`Source`] containing raw JSON data. | ||
| pub fn new(contents: impl Into<Cow<'a, str>>) -> Self { |
There was a problem hiding this comment.
Update the public docs to say JSON5, not JSON.
Both doc comments still describe strict JSON, but this type accepts JSON5 syntax. That will show up incorrectly in docs.rs and IDE hovers.
💡 Suggested change
-/// A [`Source`] containing raw JSON data.
+/// A [`Source`] containing raw JSON5 data.
@@
- /// Creates a [`Source`] containing raw JSON data.
+ /// Creates a [`Source`] containing raw JSON5 data.📝 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.
Suggested change
| /// A [`Source`] containing raw JSON data. | |
| #[derive(Clone)] | |
| pub struct Json5Source<'a> { | |
| contents: Cow<'a, str>, | |
| allow_secrets: bool, | |
| } | |
| impl<'a> Json5Source<'a> { | |
| /// Creates a [`Source`] containing raw JSON data. | |
| pub fn new(contents: impl Into<Cow<'a, str>>) -> Self { | |
| /// A [`Source`] containing raw JSON5 data. | |
| #[derive(Clone)] | |
| pub struct Json5Source<'a> { | |
| contents: Cow<'a, str>, | |
| allow_secrets: bool, | |
| } | |
| impl<'a> Json5Source<'a> { | |
| /// Creates a [`Source`] containing raw JSON5 data. | |
| pub fn new(contents: impl Into<Cow<'a, str>>) -> Self { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@confik/src/sources/json5_source.rs` around lines 5 - 14, The public doc
comments for the Json5Source type and its constructor incorrectly say "JSON" —
update them to say "JSON5" (e.g., change "A [`Source`] containing raw JSON
data." and "Creates a [`Source`] containing raw JSON data." to mention "raw
JSON5 data" and optionally note that this accepts JSON5 syntax/extensions). Edit
the doc comments on the Json5Source struct and impl/new function to reference
JSON5 and JSON5-specific parsing behavior so docs.rs and IDE hovers show the
correct format.
Comment on lines
+28
to
+35
| impl<'a> Source for Json5Source<'a> { | ||
| fn allows_secrets(&self) -> bool { | ||
| self.allow_secrets | ||
| } | ||
|
|
||
| fn provide<T: ConfigurationBuilder>(&self) -> Result<T, Box<dyn Error + Sync + Send>> { | ||
| Ok(json5::from_str(&self.contents)?) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Compare the generic `Source<T>` trait with the new JSON5 impl and existing source impls.
printf 'Trait definition:\n'
sed -n '1,20p' confik/src/sources/mod.rs
printf '\nNew JSON5 source impl:\n'
sed -n '28,40p' confik/src/sources/json5_source.rs
printf '\nExisting source impl patterns:\n'
fd '_source\.rs$' confik/src/sources -x sh -c '
echo "--- $1"
rg -n -C2 "impl.*Source|fn provide" "$1" || true
' sh {}Repository: x52dev/confik
Length of output: 5049
🏁 Script executed:
# Find the test file for json5_source and examine the assertions
fd 'json5' confik/src -type f | grep -E '(test|spec)' || echo "Checking main file for tests..."
# Look at lines 54, 60, 61 in json5_source.rs to see if there are assertions there
wc -l confik/src/sources/json5_source.rs
# Get the full content to see the assertions
cat -n confik/src/sources/json5_source.rs | grep -A5 -B5 -E '(54|60|61):'Repository: x52dev/confik
Length of output: 314
🏁 Script executed:
sed -n '50,65p' confik/src/sources/json5_source.rsRepository: x52dev/confik
Length of output: 399
Implement Source<T> here; the current impl does not match the trait.
Line 28 omits the trait generic, and Line 33 changes the trait method signature by introducing its own T. That impl cannot satisfy Source<T>, which is why the current checks fail.
🐛 Suggested fix
-impl<'a> Source for Json5Source<'a> {
+impl<'a, T> Source<T> for Json5Source<'a>
+where
+ T: ConfigurationBuilder,
+{
fn allows_secrets(&self) -> bool {
self.allow_secrets
}
- fn provide<T: ConfigurationBuilder>(&self) -> Result<T, Box<dyn Error + Sync + Send>> {
+ fn provide(&self) -> Result<T, Box<dyn Error + Sync + Send>> {
Ok(json5::from_str(&self.contents)?)
}
}🧰 Tools
🪛 GitHub Check: clippy
[failure] 28-28:
missing generics for trait sources::Source
🪛 GitHub Check: docs
[failure] 28-28:
missing generics for trait sources::Source
🪛 GitHub Check: Test / stable
[failure] 28-28:
missing generics for trait sources::Source
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@confik/src/sources/json5_source.rs` around lines 28 - 35, The impl currently
omits the trait generic and introduces a local T in provide; change the impl to
implement the generic trait by declaring impl<'a, T: ConfigurationBuilder>
Source<T> for Json5Source<'a>, keep allows_secrets(&self) as-is, and update
provide to match the trait signature fn provide(&self) -> Result<T, Box<dyn
Error + Sync + Send>> and return Ok(json5::from_str::<T>(&self.contents)?); this
ensures Json5Source implements Source<T> for the appropriate T.
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.
Fixes #122
My biggest concern here is that it's easy to typo, but I'm not sure what a better naming might be? Maybe
JsonFivebut that would likely break user expectations.Summary by CodeRabbit
New Features
Documentation