feat(config): Support for num Option Type
#95
Conversation
- can parse any among {`int`, `double`, `num`}
- corresponding new Test Cases
- updated corresponding Documentation
- fixed a few pre-existing typos
📝 WalkthroughWalkthroughAdds generic numeric option support to the config package: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User/CLI
participant NumOption
participant NumParser
participant Validator as ComparableValueOption
CLI->>NumOption: resolveNoExcept(args, config)
NumOption->>NumOption: determine generic T (double/int/num/Never)
alt T == double
NumOption->>NumParser: use NumParser<double>
else T == int
NumOption->>NumParser: use NumParser<int>
else T == num
NumOption->>NumParser: use NumParser<num>
else
NumOption->>NumParser: use NumParser<Never> (unsupported)
end
NumParser->>NumParser: parse(string) -> T or throw
NumParser->>Validator: return parsed value
Validator->>Validator: validateValue(min/max)
Validator-->>CLI: resolved typed value or errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-05-02T12:13:11.821ZApplied to files:
📚 Learning: 2025-11-19T11:55:00.345ZApplied to files:
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (4)
packages/config/README.md (1)
242-242: NumOption row matches implementation; consider clarifying default behaviorThe new
Num | NumOption<T>row correctly documents:
- Supported type arguments
T: {int,double,num}min/maxmirroring other comparable types.Given that
NumOptionwithout an explicit type argument defaults tonumand usesnum.parse, where integer-like inputs becomeintand fractional inputs becomedouble, you might optionally add a brief note to make that behavior explicit for readers.packages/config/test/config/num_option_test.dart (1)
95-144: GenericNumOptiontests capturenum.parsebehaviorThe “Given a NumOption” group (defaulting to
T == num) effectively verifies:
- Fractional input parses and is a
double.- Integer input parses and is an
int.- Non-numeric input yields errors.
This locks in the desirable “int vs double based on input” semantics for the default
NumOptioncase. If you want to make this more discoverable, you could reference this behavior briefly in the README as well.packages/config/lib/src/config/option_types.dart (2)
249-259:NumParser<T>correctly dispatches by numeric type; minor error-message polish possibleThe parser cleanly supports:
T == double→double.parseT == int→int.parseT == num→num.parse(int-or-double depending on input)This aligns with the new tests. For other
T extends num(e.g., custom numeric types),parsethrowsUnsupportedError, which is appropriate given the README documents onlyint,double, andnum.If you expect people might instantiate
NumParser<SomeCustomNum>directly, you could consider tightening the message to something like “NumParser only supports type arguments int, double, and num (got $T)” for extra clarity, but it’s not required.
261-292:NumOption<T>wiring looks correct and efficient; consider more explicit unsupported-T handlingThe
NumOption<T extends num>constructor:
- Reuses
ComparableValueOption<T>formin/maxsupport, consistent withIntOption,DateTimeOption, etc.- Selects a const
NumParser<double>,NumParser<int>, orNumParser<num>based onT, avoiding per-parse allocations.- Matches the documented table and the behavior asserted in
num_option_test.dart.For
Tvalues other thanint,double, ornum, the(…) as NumParser<T>cast will fail before parsing rather than using theUnsupportedErrorinNumParser.parse. That’s acceptable because suchTare out of scope per the docs, but if you’d like a clearer failure mode, you could add an explicit guard in the constructor that throws anUnsupportedErrorwhenTisn’t one of the three supported types.Overall, the implementation and generic design look solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/config/README.md(3 hunks)packages/config/lib/src/config/option_types.dart(2 hunks)packages/config/test/config/num_option_test.dart(1 hunks)
🔇 Additional comments (5)
packages/config/README.md (2)
38-38: Spelling/wording fix looks good“Traceability” is now spelled correctly and the bullet reads clearly; no further changes needed.
214-214: Default value callback description is accurateThe note about non-const defaults using the
fromDefaultcallback matches the API and improves the explanation.packages/config/test/config/num_option_test.dart (2)
5-54: Strong coverage forNumOption<double>semanticsThese tests nicely pin down the intended behavior:
- Fractional and integer strings both parse successfully.
- Result type is verified as
double.- Non-numeric input surfaces as errors.
This gives good confidence that the
T == doubledispatch inNumParseris wired correctly.
55-94:NumOption<int>behavior is well specifiedThe test group clearly asserts:
- Integer strings parse and yield an
intresult.- Fractional and non-numeric inputs produce errors.
That matches the
int.parsepath and ensures fractional input is rejected as intended.packages/config/lib/src/config/option_types.dart (1)
196-201: Range-check logic unchanged; spelling/variable rename is safeRenaming the local variable to
minimumand fixing the error message text doesn’t alter behavior; the min-range check still functions as before and the diagnostics are clearer.
|
@coderabbitai No worries, the types are truly exhaustive. By design, Dart only allows Reference: https://api.flutter.dev/flutter/dart-core/num-class.html |
|
Tip For best results, initiate chat on the files or code changes. @indraneel12, thank you for the clarification and the reference! You're absolutely right—since I appreciate you sharing this context about Dart's type system. The implementation looks solid! 👍 ✏️ Learnings added
🧠 Learnings used |
christerswahn
left a comment
christerswahn
left a comment
There was a problem hiding this comment.
Interesting addition!
Perhaps a DoubleOption shorthand option type can also be added?
Wrote a couple of questions.
|
|
||
| @override | ||
| T parse(final String value) { | ||
| if (T == double) return double.parse(value) as T; |
There was a problem hiding this comment.
Can this be written as a switch on T instead? If so it will perhaps let you avoid the fallback exception at the end.
There was a problem hiding this comment.
@christerswahn sir, I too like the switch format more for such use-cases. Unfortunately, Dart does not yet support switching the static value of a generic type parameter.
I've rechecked its current status: dart-lang/language#4085
If you wish to have a better alternative here, I'd be happy to know and implement it alongside.
| /// Number (int/double/num) value configuration option. | ||
| /// | ||
| /// Supports minimum and maximum range checking. | ||
| class NumOption<T extends num> extends ComparableValueOption<T> { |
There was a problem hiding this comment.
Question, should IntOption now extend NumOption? Would there be any drawbacks?
There was a problem hiding this comment.
@christerswahn sir, I agree, as I'd have had similar thoughts for the future of IntOption from a maintenance perspective.
A drawback I'm seeing, as a nitpick, is the extremely slight overhead within NumOption.parse by 2-3 condition checks and a final typecast. However, I'd vote this off as negligible.
I'm not sure about it from a design perspective; for example, in extremely rare scenarios where downstream consumer(s) rely on certain reflection-based properties for their use-cases. However, I'd again vote this off for being too pedantic (as almost any change, in general, could then be seen as a breaking change).
With respect to a potential DoubleOption, the only reason I didn't bother making it directly to resolve my issue is because "Double Option" sounds odd to me even within that context. I'm not too concerned about it tho.
Please declare your decision, and I'd adapt the code accordingly:
- whether or not to refactor
IntOption - whether or not to add
DoubleOption
- handles the `Never` case - better documentation
2 participants
Resolves #94
Consumers of this package can now avail built-in support for
numamong Option Types via the newNumOptionclass.Old Behavior
As described in the referenced issue, this feature is absent in the current version of this package.
New Behavior
The desired functionality is achieved, and all tests pass successfully. It is now possible to parse
intordoubleornumviaNumOption.Example
Demo
Summary by CodeRabbit
Documentation
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.