fix(config): Created a public OptionResolution interface #91
Conversation
Closes [config] `OptionResolution` is not exported #72
📝 WalkthroughWalkthroughRefactors option resolution by making Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Configuration as Configuration
participant Options as Option implementations
participant Impl as OptionResolutionImpl
Note over Options,Configuration: Resolution values are produced as OptionResolutionImpl
Caller->>Configuration: resolve(...)
Configuration->>Options: option.resolveValue(...)
Options->>Impl: instantiate OptionResolutionImpl(value / noValue / error)
Impl-->>Configuration: OptionResolutionImpl
Configuration-->>Caller: Map<Option, OptionResolutionImpl>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-06-12T14:55:38.006ZApplied to files:
🔇 Additional comments (1)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/config/lib/src/config/configuration.dart (1)
7-372: UseOptionResolutionwhen aggregating group values.We still build the group resolution map with
OptionResolutionImpl, which meansOptionGroup.validateValuescan’t actually switch to the new publicOptionResolutioninterface without hitting generic invariance issues. Please import the interface and type the map (and helper) accordingly so overrides only depend on the exported contract.-import 'option_resolution_impl.dart'; +import 'option_resolution.dart'; +import 'option_resolution_impl.dart'; @@ - final optionGroups = <OptionGroup, Map<O, OptionResolutionImpl>>{}; + final optionGroups = <OptionGroup, Map<O, OptionResolution>>{}; @@ - void _validateGroups( - final Map<OptionGroup, Map<O, OptionResolutionImpl>> optionGroups, + void _validateGroups( + final Map<OptionGroup, Map<O, OptionResolution>> optionGroups, ) {packages/config/lib/src/config/options.dart (1)
329-373: Return the public interface, not the internal impl.
resolveValueis part of the exported API, so switching its return type toOptionResolutionImpl<V>leaks the internal implementation class you’re trying to keep hidden. Consumers still can’t rely solely on the newOptionResolutioninterface, and any overrides are now forced to depend on the internal impl, which defeats the goal of this PR. Keep the public signature on the interface and useOptionResolutionImplonly inside the method body.- OptionResolutionImpl<V> resolveValue( + OptionResolution<V> resolveValue(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/config/lib/src/config/config.dart(1 hunks)packages/config/lib/src/config/configuration.dart(9 hunks)packages/config/lib/src/config/option_resolution.dart(1 hunks)packages/config/lib/src/config/option_resolution_impl.dart(1 hunks)packages/config/lib/src/config/options.dart(8 hunks)packages/config/lib/src/config/source_type.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: indraneel12
Repo: serverpod/cli_tools PR: 0
File: :0-0
Timestamp: 2025-10-10T08:50:09.902Z
Learning: PR #71 in serverpod/cli_tools adds support for displaying OptionGroup names in CLI usage output. The implementation does NOT include fallback group names or numbered defaults for anonymous groups - it only displays the group.name when present and shows groupless options first without any header.
📚 Learning: 2025-08-07T07:55:17.269Z
Learnt from: christerswahn
Repo: serverpod/cli_tools PR: 57
File: packages/config/test/better_command_runner/default_flags_test.dart:1-1
Timestamp: 2025-08-07T07:55:17.269Z
Learning: In the `config` package, `better_command_runner.dart` is intentionally kept as a separate import (`package:config/better_command_runner.dart`) rather than being re-exported through the main `packages/config/lib/config.dart` barrel file. This separation is by design according to the package maintainer christerswahn.
Applied to files:
packages/config/lib/src/config/config.dartpackages/config/lib/src/config/configuration.dart
📚 Learning: 2025-06-12T14:55:38.006Z
Learnt from: christerswahn
Repo: serverpod/cli_tools PR: 47
File: lib/src/config/options.dart:552-567
Timestamp: 2025-06-12T14:55:38.006Z
Learning: The codebase relies on a recent version of `package:args` where `ArgParser.addFlag` accepts the `hideNegatedUsage` parameter.
Applied to files:
packages/config/lib/src/config/options.dart
nielsenko
left a comment
nielsenko
left a comment
There was a problem hiding this comment.
LGTM 👍
I would also have been okay with just making the existing OptionResolution class public.
The class
OptionResolutionwas internal, yet used in a public and overridable methodOptionGroup.validateValues.This PR breaks out a public interface now called
OptionResolutionand exported from the package. The internal implementation has been moved toOptionResolutionImpl.Closes [config]
OptionResolutionis not exported #72Summary by CodeRabbit
Documentation
Refactor