Repository navigation
Conversation
…m (2/3) - Add optional parse-time value validator hook on `Option` (`validatorOf` / `validateOptionValue` in `lib/src/option.dart` and `Parser._validateAllowed` in `lib/src/parser.dart`). - Add `IntOptionDescriptor` and `DefaultedIntOptionDescriptor` with parse-time integer validation and read-time `FormatException` fallback. - Add `CliEnum`, `EnumOptionDescriptor`, and `DefaultedEnumOptionDescriptor` with automatic `allowed` and `allowedHelp` derivation.
| // for details. All rights reserved. Use of this source code is governed by a | ||
| // BSD-style license that can be found in the LICENSE file. | ||
|
|
||
| /// An interface for enums that provide custom CLI names and help text. |
There was a problem hiding this comment.
How? Why? What does this mean?
Are the enum values related? Are they the valid values of a single property (that would make sense),
or are they independent unrelated properties for some command line?
(It's the former. Not clear from this doc.)
| } | ||
|
|
||
| /// A private base class providing shared integer parsing and registration | ||
| /// infrastructure for [IntOptionDescriptor] and [DefaultedIntOptionDescriptor]. |
There was a problem hiding this comment.
Document classes by documenting an instance.
Don't start with "A private base class". That's documenting the declaration, not its meaning, and it says all that right below: base class _... (aka private base class).
| } | ||
| } | ||
|
|
||
| /// A descriptor for single-value integer options. |
There was a problem hiding this comment.
See below. I'm guessing:
/// Descriptor for a single-value integer option.| } | ||
| } | ||
|
|
||
| /// A descriptor for single-value integer options that have a non-null default |
There was a problem hiding this comment.
Singular descriptor for (single-value) plural options that have a singular defaul value.
I'm confused!
Is an instance of this class a descriptor for multiple options?
Document classes by documenting a single instance.
I'm guessing:
/// Descriptor for a single-value integer option with a non-`null` default value.Does this mean that IntOptionDescriptor has a null default value?
Or is it that it has a nullable/optional default value, so you can use it whether you know that there is a the default value or not.
Would it make sense to:
a. Remove super.defaultsTo from IntOptionDescriptor?
b. Combine these two into class IntOptionDescriptor<Default extends int?>, required Default super.defaultsTo constructor parameter, and Default getValue(...)? It makes the defaultsTo required even when it's nullable, because we can't abstract over optionality. I think it may be worth it.
| } | ||
| } | ||
|
|
||
| /// A private base class providing shared enum parsing and registration |
| OptionType get _optionType => OptionType.single; | ||
|
|
||
| @override | ||
| Object? get _rawDefaultsTo => |
There was a problem hiding this comment.
Doesn't feel very "raw" when it processes the actual defaultsTo value.
Maybe _stringDefaultsTo?
| defaultsTo != null ? _formatName(defaultsTo! as T) : null; | ||
|
|
||
| @override | ||
| List<String> get allowed => values.map(_formatName).toList(); |
There was a problem hiding this comment.
You know I love literals:
=> [for (final v in values) _formatName(v)];Otherwise, maybe do .toList(growable: false) to save a little memory.
Also, is this called more than once? Then it may be worth caching the result.
(Or not, if it's short.)
| } | ||
|
|
||
| final defaultString = | ||
| defaultsTo != null ? _formatName(defaultsTo! as T) : null; |
| return valueParser!(raw); | ||
| } | ||
| for (final val in values) { | ||
| if (_formatName(val) == raw || val.name == raw) { |
There was a problem hiding this comment.
This treats val.name as a valid representation of the val even if it has a cliName.
If you have
enum WhatnotX(final String cliName, final String helpText) implements CliEnum {
foo('xfoo', 'the xfoo'),
bar('xbar', 'the xbar'),
}then _parse('foo') will return the WhatnotX.foo value, even if it's not the correct name.
Maybe do:
if (raw == _firmatName(val) ?? val.name) { ... }Then you only compare to one name, and only use val.name as default if _formatName gives you nothing.
(But then, why does _formatName not return val.name at the end? That would ensure consistency.)
| final defaultPri = priorityOpt.getValue(defaultResults); | ||
| expect(defaultPri, equals(TestPriority.p1)); | ||
|
|
||
| final customResults = parser.parse(['-p', 'P0']); |
There was a problem hiding this comment.
Check that parser.parse(['-p', 'p0']) does not parse to TestPriority.p0!
- Shorten integer and enum descriptor names (`IntDescriptor`, `DefaultedIntDescriptor`, `EnumDescriptor`, `DefaultedEnumDescriptor`) and omit `defaultsTo` from nullable constructors. - Enforce mapped CLI names in `_EnumDescriptorBase._parse`, include raw input as `source` in `FormatException`, privatize `_nameMapper`, and drop `valueParser` and `CliEnum.allowedHelp`. - Apply instance-centric Dartdoc phrasing and Dart 3 pattern promotion across `cli_enum.dart` and `option_descriptor.dart`.
kevmoo
changed the title
|
Updated
|
Adds integer and enum option descriptors with parse-time integer validation and
CliEnumallowed-value metadata, stacked on #1012 and withholding the publicpackage:args/typed.dartexport until #1017.OptionandParser._validateAllowedadd a package-private validator hook soIntDescriptorandDefaultedIntDescriptorreject non-integer CLI inputs at parse time with anArgParserException(converted toUsageExceptionbyCommandRunner).CliEnum,EnumDescriptor, andDefaultedEnumDescriptorderiveallowedandallowedHelpfrom enum values (or an optionalnameMapper) and match parsed CLI values strictly against the formatted CLI name.OptionBundle,Option.copyTo,typed.dart).