Remove clap::ValueEnum from domain config enums (#320)#366
Conversation
`ColourPolicy`, `SpinnerMode`, and `OutputFormat` derived `clap::ValueEnum` purely as a `FromStr` implementation detail, coupling the domain configuration schema to the Clap parsing layer. Implement `FromStr` with direct case-insensitive matching (mirroring the previous `ValueEnum::from_str(s, true)` semantics) and drop the derives. `parse_value_enum` in the parsing layer now dispatches via `FromStr` instead of `ValueEnum`, so the Clap-facing logic stays centralised in `parsing.rs` as the issue proposed. `Theme` retains its derive: it is parsed through `ThemePreference::parse_raw` and was outside the issue's scope. The BDD configuration-preference steps now use `Display`/`FromStr` for the three domain enums instead of `to_possible_value`/`from_str`.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideDecouples configuration enums from clap::ValueEnum by implementing custom FromStr parsing and updating CLI parsing and BDD tests to use FromStr/Display instead of ValueEnum/to_possible_value, while preserving existing behavior and user-facing strings. Sequence diagram for CLI enum parsing via FromStrsequenceDiagram
actor User
participant Clap
participant parsing_rs as parse_value_enum
participant EnumFromStr as FromStr_impl
participant Parser as parser_validation
User->>Clap: provide CLI arg s
Clap->>parsing_rs: parse_value_enum(localizer, s, spec)
parsing_rs->>EnumFromStr: s.parse::<Enum>()
alt parse ok
EnumFromStr-->>parsing_rs: Ok(Enum)
parsing_rs-->>Clap: Ok(Enum)
Clap-->>User: configuration applied
else parse error
EnumFromStr-->>parsing_rs: Err(String)
parsing_rs->>Parser: parser::validation_message(localizer, spec, s)
Parser-->>parsing_rs: localized error
parsing_rs-->>Clap: Err(localized error)
Clap-->>User: display error message
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary
Closes #320
Decouples the domain configuration schema from the Clap parsing layer:
src/cli/config.rs: drop#[derive(clap::ValueEnum)]fromColourPolicy,SpinnerMode, andOutputFormat; eachFromStrnow matches kebab-case names case-insensitively, mirroring the previousValueEnum::from_str(s, true)behaviour exactly.src/cli/parsing.rs:parse_value_enumdispatches viaFromStrinstead ofValueEnum, keeping the Clap-facing dispatch centralised inParseEnumSpecas proposed.tests/bdd/steps/configuration_preferences.rs: the steps for the three enums useDisplay/str::parseinstead ofto_possible_value/ValueEnum::from_str.Themekeeps its derive — it parses throughThemePreference::parse_rawand is outside this issue's scope.No behaviour change: clap's derived parser inference falls back to
FromStr, and the runtime parsers were already the localisedparse_*functions.Validation
make check-fmt/make lint/make test— pass (37 suites, including all CLI parsing and configuration-preference BDD scenarios)🤖 Generated with Claude Code
Summary by Sourcery
Decouple configuration enums from clap by parsing them via
FromStrinstead ofclap::ValueEnumwhile preserving existing CLI behaviour.Enhancements:
FromStrparsing forColourPolicy,SpinnerMode, andOutputFormatenums with case-insensitive, kebab-case matching and remove theirValueEnumderives.FromStr-based parsing rather thanclap::ValueEnumto reduce coupling to the argument parser.Tests:
DisplayandFromStr/str::parseinstead ofValueEnumutilities, keeping test behaviour aligned with the new parsing model.