-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Provide usage help text for set of options #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThis update refactors the configuration option system by extracting option types, parsers, and validation logic into separate modules, introducing new abstractions for option definitions and brokers, and reorganizing exports and imports accordingly. Several new files are added, and existing files are updated to use the new modular structure. Additional tests are included for usage string reporting. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ArgParser
participant Configuration
participant OptionDefinition
participant ConfigurationBroker
participant ValueParser
User->>Configuration: Initialize with OptionDefinitions
Configuration->>ArgParser: Prepare options for parsing
User->>Configuration: Request value for option
Configuration->>OptionDefinition: resolveValue()
OptionDefinition->>ConfigurationBroker: (if needed) valueOrNull(key, config)
OptionDefinition->>ValueParser: parse(rawValue)
OptionDefinition->>OptionDefinition: validateOptionValue(parsedValue)
OptionDefinition-->>Configuration: Return resolved/validated value
sequenceDiagram
participant User
participant Configuration
participant OptionDefinition
User->>Configuration: Request usage
Configuration->>OptionDefinition: Get usage string
OptionDefinition-->>Configuration: Return usage/help text
Configuration-->>User: Return usage/help text
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (4)
lib/src/config/exceptions.dart (1)
4-15: Consider extendingExceptioninstead ofError
InvalidOptionConfigurationErrorrepresents a recoverable, user-level configuration mistake rather than a programming bug. ExtendingException(or creating a subtype of it) is more idiomatic in Dart for signalling input / configuration problems, whileErroris intended for unrecoverable programming errors (e.g. assertion failures).-class InvalidOptionConfigurationError extends Error { +class InvalidOptionConfigurationError implements Exception {Keeping the rest of the class unchanged preserves the API and
toStringformatting.lib/src/config/option_types.dart (2)
41-63: Trim elements when splitting multi-string input
MultiStringOptionsplits on commas but keeps any surrounding whitespace, so"a, b ,c "yields['a', ' b ', 'c '].
Add.trim()when mapping, or document the current behaviour explicitly.
196-203: Typo:mininumvariable name
final mininum = min;– the misspelling makes the code look sloppy and complicates grepping later. Rename tominimum.lib/src/config/options.dart (1)
245-252: ConvertIterabletoListbefore passing toallowed
argParser.addOptionexpectsList<String>?; pass.toList()to avoid implicit cast issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/src/config/config.dart(1 hunks)lib/src/config/config_parser.dart(1 hunks)lib/src/config/config_source_provider.dart(1 hunks)lib/src/config/configuration.dart(4 hunks)lib/src/config/configuration_broker.dart(1 hunks)lib/src/config/exceptions.dart(1 hunks)lib/src/config/file_system_options.dart(1 hunks)lib/src/config/multi_config_source.dart(1 hunks)lib/src/config/option_groups.dart(1 hunks)lib/src/config/option_types.dart(1 hunks)lib/src/config/options.dart(1 hunks)test/config/configuration_test.dart(1 hunks)
🔇 Additional comments (14)
lib/src/config/file_system_options.dart (1)
5-5: Import update looks correctThe switch from
configuration.darttooptions.dartaligns the file with the new modular structure and resolves theOptionDefinitionsymbol without introducing any side-effects.lib/src/config/config_source_provider.dart (1)
4-4: Import aligns with refactorAdding
options.dartis required after relocatingOptionDefinition; build will fail without it. No further changes needed.lib/src/config/config_parser.dart (1)
7-9: New imports are necessary and correctly scoped
configuration_broker.dartsupplies theConfigurationBrokertype used inparse, andoption_types.dartprovides the concrete option classes referenced throughout the file. No redundancy or unused-import warnings expected.lib/src/config/option_groups.dart (1)
1-4: Imports match the new package boundariesReplacing the broad
configuration.dartimport with fine-grainedexceptions.dartandoptions.dartkeeps dependencies minimal and prevents circular imports. Looks good.lib/src/config/multi_config_source.dart (1)
3-4: LGTM! Necessary imports for the ConfigurationBroker implementation.The imports correctly support the
MultiDomainConfigBrokerclass which implementsConfigurationBroker<O>and usesOptionDefinitionas a type constraint.test/config/configuration_test.dart (2)
125-133: Good test coverage for the new usage feature on option lists.The test correctly validates that the
usageproperty on a list of options returns the expected help text format, showing thefromDefaultfunction name in the usage string.
135-145: Good test coverage for the usage feature on Configuration instances.The test validates that the
usagegetter on a resolvedConfigurationinstance returns the same help text as when accessed directly from the option list, ensuring consistency.lib/src/config/configuration_broker.dart (1)
1-12: Well-designed interface for configuration value resolution.The
ConfigurationBrokerinterface provides a clean abstraction for dynamic value resolution with appropriate type constraints and clear documentation. The design allows for flexible implementations that can resolve values from multiple sources and handle dependencies between options.lib/src/config/config.dart (1)
6-12: Appropriate exports for the new modular configuration system.The added exports correctly expose the new configuration infrastructure:
configuration_broker.dart- for dynamic value resolutionexceptions.dart- for error handlingoption_types.dart- for typed option implementationsoptions.dart- for the option frameworkThe ordering logically places type definitions before the framework that uses them.
lib/src/config/configuration.dart (4)
5-7: Necessary imports for the refactored configuration system.The imports correctly bring in the new modular components that handle option definitions, value resolution, and error handling.
11-42: Excellent documentation improvement with comprehensive example.The updated documentation provides a clear, practical example that demonstrates:
- Best practice of using enums for option definitions
- Integration with command-line arguments and environment variables
- Complete runnable code that developers can easily adapt
This significantly improves the developer experience for new users of the configuration system.
112-113: Clean implementation of the usage getter.The getter appropriately delegates to the options collection, providing access to the usage help text functionality as intended by the PR objectives.
256-256: Good refactoring - delegating option logic to the option objects.The changes correctly delegate value resolution and validation to the option objects themselves, following object-oriented principles and reducing the Configuration class's responsibilities. This improves modularity and maintainability.
Also applies to: 305-305
lib/src/config/options.dart (1)
404-413: Reliance onArgResults.option()API
args.option(argOptName)is likewise not in the public API. Verify that an extension providing it exists; otherwise this will fail at compile time.
SandPod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
Adds methods to get usage help texts for a set of options, and for the options of a resolved configuration.
(This still uses the usage writer of ArgParser, which is still used internally. In the future this should be replaced.)
Added better usage examples in the doc comments.
Some code restructuring to split up large source files.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests