feat(humantime): add support for Option<Duration> with serde_humantime#375
feat(humantime): add support for Option<Duration> with serde_humantime#375
Conversation
📝 WalkthroughWalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📝 Coding Plan
Comment Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
Whilst Duration itself has an Option<Duration> builder, which serde_humantime already handles, Option<Duration> uses OptionBuilder<Duration>, which serde_humantime does not support
95a47bb to
c94b381
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
confik/tests/third_party.rs (1)
7-26: Test coverage is limited to the happy path.The test validates parsing a present duration value but doesn't cover:
- Missing
timeoutfield (should result inNone)- Behavior when the field is explicitly absent
Consider adding test cases for these scenarios to ensure the
OptionBuilderhandling works correctly for all cases.🧪 Suggested additional test cases
#[test] fn from_humantime_option_missing() { #[derive(Debug, PartialEq, Eq, Configuration)] struct Config { #[confik(forward(serde(with = "confik::humantime::option")))] timeout: Option<Duration>, } let config = ConfigBuilder::<Config>::default() .override_with(TomlSource::new("")) .try_build() .unwrap(); assert_eq!(config, Config { timeout: None }); }🤖 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 7 - 26, Add negative-path tests for the humantime option handling: create tests named like from_humantime_option_missing and from_humantime_option_explicit_none that use the same Config struct (with #[confik(forward(serde(with = "confik::humantime::option")))] timeout: Option<Duration>) and build via ConfigBuilder::<Config>::default().override_with(TomlSource::new(...)).try_build().unwrap(); for the missing case pass an empty source ("" ) and assert Config { timeout: None }, and for the explicit-absent case pass a TOML that sets timeout = none (or deletes the key) and assert None; add these new tests alongside from_humantime_option to ensure OptionBuilder and the serde wrapper handle absent/explicit-null correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@confik/tests/third_party.rs`:
- Around line 7-26: Add negative-path tests for the humantime option handling:
create tests named like from_humantime_option_missing and
from_humantime_option_explicit_none that use the same Config struct (with
#[confik(forward(serde(with = "confik::humantime::option")))] timeout:
Option<Duration>) and build via
ConfigBuilder::<Config>::default().override_with(TomlSource::new(...)).try_build().unwrap();
for the missing case pass an empty source ("" ) and assert Config { timeout:
None }, and for the explicit-absent case pass a TOML that sets timeout = none
(or deletes the key) and assert None; add these new tests alongside
from_humantime_option to ensure OptionBuilder and the serde wrapper handle
absent/explicit-null correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c698f400-f4c8-4df5-9bb8-e48b55740000
📒 Files selected for processing (5)
confik/CHANGELOG.mdconfik/Cargo.tomlconfik/src/lib.rsconfik/src/third_party.rsconfik/tests/third_party.rs
✅ Files skipped from review due to trivial changes (1)
- confik/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- confik/Cargo.toml
Whilst Duration itself has an Option builder, which serde_humantime already handles, Option uses OptionBuilder, which serde_humantime does not support
Summary by CodeRabbit
New Features
Tests
Documentation