feat: Migrated the Config library to cli_tools #32
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis update introduces a comprehensive, extensible configuration management system for Dart CLI tools. It adds a suite of configuration option classes, parsers, and brokers to handle command-line arguments, environment variables, and configuration files (JSON/YAML). The system supports typed options, validation, precedence rules, multi-domain sources, and grouping (including mutual exclusion). Numerous test files validate parsing, precedence, error handling, usage string generation, file/directory options, and performance. The update also expands dependencies and disables a lint rule. No changes were made to existing public API signatures; new files and exports were added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigParser
participant ArgParser
participant OptionDefinitions
participant Configuration
participant ConfigSourceProvider
participant ConfigurationSource
User->>ConfigParser: addFlag/addOption(...)
ConfigParser->>OptionDefinitions: Register option definitions
User->>ConfigParser: parse(args, env, configSource, ...)
ConfigParser->>ArgParser: Parse command-line args
ArgParser-->>ConfigParser: Parsed values
ConfigParser->>Configuration: Resolve values (args, env, config, custom, default)
Configuration->>ConfigSourceProvider: (if needed) getConfigSource(cfg)
ConfigSourceProvider->>ConfigurationSource: Provide config data
ConfigurationSource-->>Configuration: Value for key
Configuration-->>ConfigParser: Resolved Configuration (with errors if any)
ConfigParser-->>User: ConfigResults (option values, errors, rest args)
Possibly related PRs
Suggested reviewers
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:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (30)
analysis_options.yaml (1)
3-5: Scope down disabling ofunnecessary_final
Turning offunnecessary_finalglobally can hide valuable linter suggestions for removing redundantfinalkeywords across the entire codebase. It’s often better to limit this override to specific files or directories (e.g., generated code or tests), or to use inline// ignore: unnecessary_finalcomments for isolated cases.test/config/date_parsing_test.dart (1)
1-117: Consider adding edge case date-time tests.While the current test coverage is good, consider adding tests for these edge cases:
- Invalid dates (e.g., February 30)
- Different time zones beyond UTC
- Leap years
- Daylight saving time transitions
- Extreme dates (very old or future dates)
These additional test cases would further strengthen the robustness of the DateTimeParser.
lib/src/config/json_yaml_document.dart (2)
17-22: Consider handling empty YAML string consistently with empty JSONThe
fromJsonconstructor properly handles empty JSON strings by setting the document to null, but thefromYamlconstructor lacks similar handling. This could lead to inconsistent behavior or potential errors when parsing empty YAML content.JsonYamlDocument.fromYaml(final String yamlSource) - : _document = yamlDecode(yamlSource); + : _document = yamlSource.isEmpty ? null : yamlDecode(yamlSource);
23-27: Add method to check if a pointer existsConsider adding a method to check if a JSON pointer exists before attempting to retrieve its value. This would help users avoid null checks when they want to verify existence.
+ /// Returns true if the given JSON pointer resolves to a value in the document. + bool hasValueAtPointer(final String pointerKey) { + final pointer = JsonPointer(pointerKey); + try { + pointer.read(_document); + return true; + } catch (_) { + return false; + } + }lib/src/config/option_groups.dart (1)
27-34: Enhance error message with specific conflicting optionsThe current error message lists all options in the group, but it would be more helpful to identify which specific options have values and are conflicting.
if (providedCount > 1) { - final opts = optionResolutions.keys.map((final o) => o.option); - return 'These options are mutually exclusive: ${opts.join(', ')}'; + final providedOpts = optionResolutions.entries + .where((entry) => allowDefaults ? entry.value.isSpecified : entry.value.hasValue) + .map((entry) => entry.key.option) + .join(', '); + return 'Mutually exclusive options provided: $providedOpts'; }lib/src/config/config_source.dart (2)
7-10: Consider return type consistencyThe interface method
valueOrNullreturnsObject?, but the implementation inMapConfigSourcereturnsString?. While this is type-safe (String is a subtype of Object), it might confuse users about the expected return type.Either specify in the documentation that implementations may return more specific types, or consider making the interface method generic:
abstract interface class ConfigurationSource { /// {@macro config_source.valueOrNull} - Object? valueOrNull(final String key); + T? valueOrNull<T>(final String key); }
12-22: Document type limitation of MapConfigSourceThe
MapConfigSourceimplementation only supports string values. Consider documenting this limitation or providing a more flexible implementation./// Simple [ConfigurationSource] adapter for a [Map<String, String>]. +/// +/// Note: This implementation only supports string values. For more complex +/// data types, consider using a custom [ConfigurationSource] implementation. class MapConfigSource implements ConfigurationSource {test/config/args_compatibility/allow_anything_test.dart (1)
1-2: Document reason for skipping and alternative approachesThe test file is skipped with a note that
ArgParser.allowAnything()is not supported, but it doesn't explain why or what the alternative approach is in the new configuration system.-@Skip('ArgParser.allowAnything() not supported') +@Skip('ArgParser.allowAnything() not supported - Use ConfigParser with appropriate options instead') library;test/config/args_compatibility/parse_performance_test.dart (3)
42-42: Consider using ConfigParser type instead of ArgParser in the method signature.While the code works due to inheritance, using
ConfigParserinstead ofArgParserin the method signature would better reflect the actual implementation being tested. This would make the test more explicit about what's being tested.-void _testParserPerformance(ArgParser parser, String string) { +void _testParserPerformance(ConfigParser parser, String string) {
49-50: Return types should match the parser implementation.These methods return
ArgResultsdespite the parser potentially being aConfigParser. Consider updating the return types to match the output ofparser.parse()for better type safety.- ArgResults baseAction() => parser.parse(baseList); - ArgResults largeAction() => parser.parse(largeList); + var baseAction() => parser.parse(baseList); + var largeAction() => parser.parse(largeList);
73-77: Consider using microseconds for more precise measurements.For precise performance measurements, especially with fast operations, using microseconds instead of milliseconds can provide better resolution and reduce the likelihood of false positives due to rounding.
int _time(void Function() function) { var stopwatch = Stopwatch()..start(); function(); - return stopwatch.elapsedMilliseconds; + return stopwatch.elapsedMicroseconds ~/ 1000; // Convert to milliseconds after measuring }test/config/args_compatibility/trailing_options_test.dart (2)
23-25: Function signature could be improved for clarity.The function signature doesn't indicate what type of exception is expected to be thrown, which makes it less clear what the function is validating.
- void expectThrows(List<String> args, String arg) { + void expectThrows(List<String> args, String expectedInvalidArg) { throwsFormat(parser, args, reason: 'with allowTrailingOptions: true'); }
82-82: Document the feature gap in command support.Several tests are skipped due to commands not being supported. It would be helpful to add a TODO comment or documentation about when command support will be implemented.
- }, skip: 'commands not supported'); + }, skip: 'commands not supported yet - will be added in future iterations');lib/src/config/configuration_parser.dart (1)
59-68: Consider adding stronger typing for valueOrNull.The
_JyConfigSourceimplementation ofvalueOrNullreturnsObject?, which might lead to type casting issues when specific types are expected. Consider enhancing the API to provide more type safety.class _JyConfigSource implements ConfigurationSource { final JsonYamlDocument _jyDocument; _JyConfigSource(this._jyDocument); @override Object? valueOrNull(final String pointerKey) { return _jyDocument.valueAtPointer(pointerKey); } + + @override + T? valueOrNullAs<T>(final String pointerKey) { + final value = valueOrNull(pointerKey); + return value is T ? value : null; + } }Note: This suggestion assumes
ConfigurationSourceinterface has or can be updated to include avalueOrNullAs<T>method.lib/src/config/option_resolution.dart (3)
3-14: Consider adding documentation for the OptionResolution class.The
OptionResolutionclass serves an important role in the configuration system, but lacks comprehensive documentation explaining its purpose and usage. Adding class-level documentation would help users understand how this class fits into the overall architecture.+/// Represents the resolution state of a configuration option. +/// +/// This class encapsulates: +/// - The original string value parsed from the source +/// - The typed value after conversion +/// - Any error encountered during processing +/// - The source of the value (command line, environment, etc.) +/// +/// It provides methods to check the state of the resolution and +/// to create modified copies with updated values or errors. final class OptionResolution<V> { final String? stringValue; final V? value; final String? error; final ValueSourceType source;
47-54: Add examples for hasValue, isSpecified, and isDefaultThe boolean getter methods have good summaries but would benefit from examples to clarify their subtle differences and use cases.
/// Whether the option has a proper value (without errors). + /// + /// Example: `--name=value` or environment variable was set successfully. bool get hasValue => source != ValueSourceType.noValue && error == null; /// Whether the option has a value that was specified explicitly (not default). + /// + /// Example: Value provided via CLI or environment, not from default values. bool get isSpecified => hasValue && source != ValueSourceType.defaultValue; /// Whether the option has the default value. + /// + /// Example: Option wasn't specified explicitly and is using its default value. bool get isDefault => hasValue && source == ValueSourceType.defaultValue;
16-31: Consider adding equals and hashCode for comparison supportSince
OptionResolutionis an immutable value class, adding==operator andhashCodewould make it easier to compare instances, which could be useful in tests and for caching.+ @override + bool operator ==(Object other) { + if (identical(this, other)) return true; + return other is OptionResolution<V> && + other.stringValue == stringValue && + other.value == value && + other.error == error && + other.source == source; + } + + @override + int get hashCode => Object.hash(stringValue, value, error, source);lib/src/config/config_source_provider.dart (1)
16-42: Effective implementation with appropriate caching strategyThe
OptionContentConfigProviderimplementation provides a practical way to use a string option's value as configuration content. The caching mechanism is properly implemented, reducing redundant parsing operations.Two suggestions for enhancement:
- Consider adding a mechanism to invalidate the cache if needed (for long-running applications where configuration might change)
- Consider adding exception handling during parsing or documenting that exceptions are propagated
@override ConfigurationSource getConfigSource(final Configuration<O> cfg) { var provider = _configProvider; if (provider == null) { final optionValue = cfg.optionalValue(contentOption) ?? ''; + try { provider = ConfigurationParser.fromString( optionValue, format: format, ); _configProvider = provider; + } catch (e) { + // Either rethrow with more context or handle appropriately + throw FormatException('Failed to parse ${format.name} content from option ${contentOption.argName}: $e'); + } } return provider; }test/config/args_compatibility/parse_test.dart (2)
576-578: Remove commented-out codeThere's a commented-out line of code that appears to be leftover from development. Either restore it if it's needed for a test case or remove it completely.
parser.addMultiOption('define', defaultsTo: ['0']); - // var args = parser.parse(['']); var args = parser.parse([]);
708-709: Remove commented-out codeAnother instance of commented-out code that should be cleaned up.
parser.addMultiOption('define', defaultsTo: ['0']); - // var args = parser.parse(['']); var args = parser.parse([]);lib/src/config/file_system_options.dart (1)
22-75: Robust validation logic with clear error messagesThe DirOption class provides comprehensive validation for directory paths based on the specified PathExistMode. The error messages are descriptive and helpful for users.
One minor suggestion for improvement would be to add more detailed documentation about the behavior of each PathExistMode case.
Consider expanding the class documentation to include specific examples of when to use each PathExistMode case.
lib/src/config/multi_config_source.dart (1)
64-94: Robust value resolution logic with clear error handlingThe valueOrNull method implements a thoughtful approach to finding and extracting configuration values based on pattern matching. The error message for unmatched domains is clear and helpful.
One potential improvement might be to add logging for diagnostic purposes when a domain match is found or when extraction patterns are applied.
Consider adding debug logging to help diagnose configuration resolution issues, particularly for complex regex patterns.
lib/src/config/config_parser.dart (1)
24-30: Usingimplements ArgParsercouples you to every future API change
ConfigParsermanually re‑implements most ofArgParser. Any new member added upstream will break compilation here.
Consider switching toclass ConfigParserwithdelegatepattern instead (wrapArgParserbut don’timplements) or create a mix‑in to selectively expose only what you need.This will greatly reduce maintenance overhead.
test/config/args_compatibility/args_test.dart (1)
103-106: Minor typo in test description
'allows explict null value for "abbr"'→'allows explicit null value for "abbr"'.- test('allows explict null value for "abbr"', () { + test('allows explicit null value for "abbr"', () {test/config/args_compatibility/command_runner_test.dart (1)
1-2: Double‑check the need for the entire (skipped) test file
@Skip('CommandRunner not supported')disables the whole suite. IfConfigParserwill never supportCommandRunner, consider removing the file to avoid dead code and reduce test execution time.
Alternatively, keep it but add a short comment explaining why it is retained (e.g., planned future support).test/config/configuration_test.dart (1)
7-10: Spelling typo in test description
abbrevation➔abbreviation.
While purely cosmetic, fixing typos in descriptions makes failing‑test output easier to grep for and improves long‑term readability.- group('Given invalid configuration abbrevation without full name', () { + group('Given invalid configuration abbreviation without full name', () {test/config/args_compatibility/test_utils.dart (1)
48-66: Duplicate command names may mask each other
FooCommand,ValueCommand, andAsyncValueCommandall expose
name = 'foo'.
When several of these commands are added to the sameCommandRunner
only one will actually be reachable; the rest will silently be
over‑written in the underlyingMap<String, Command>.
If this is intentional, consider adding an explicit comment; otherwise,
give each test command a uniquename.lib/src/config/options.dart (3)
149-154: RawComparablegeneric triggers analysis warning
V extends Comparableimplicitly meansComparable<dynamic>, which
produces astrict_raw_type/always_specify_type_argumentslint in
the latest SDKs.-class ComparableValueOption<V extends Comparable> extends ConfigOptionBase<V> { +class ComparableValueOption<V extends Comparable<dynamic>> + extends ConfigOptionBase<V> {This keeps the public API identical while silencing the warning.
181-183: Typo in local variable nameTiny nit –
mininum➔minimum. Although harmless, consistent naming
helps readability.- final mininum = min; - if (mininum != null && value.compareTo(mininum) < 0) { + final minimum = min; + if (minimum != null && value.compareTo(minimum) < 0) {
347-350:_unitStr()ignores exact multiples (e.g. 1 day 0 h) for non‑first unitCurrent logic only appends a unit when
absValue % mod > 0, so an exact
multiple of the higher unit prints nothing for the lower one – good.
However, for the first unit (days) we usually want the full
absolute value, not a remainder. If you adopt the earlier suggestion,
consider makingmodnullable and treatingnullas “no modulo”:static String _unitStr(int value, int? mod, String unit) { final abs = value.abs(); if (mod == null) return abs == 0 ? '' : '$abs$unit'; return abs % mod > 0 ? '${abs % mod}$unit' : ''; }This generalises the formatter and avoids special‑casing at call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
analysis_options.yaml(1 hunks)lib/config.dart(1 hunks)lib/src/config/config.dart(1 hunks)lib/src/config/config_parser.dart(1 hunks)lib/src/config/config_source.dart(1 hunks)lib/src/config/config_source_provider.dart(1 hunks)lib/src/config/configuration.dart(1 hunks)lib/src/config/configuration_parser.dart(1 hunks)lib/src/config/file_system_options.dart(1 hunks)lib/src/config/json_yaml_document.dart(1 hunks)lib/src/config/multi_config_source.dart(1 hunks)lib/src/config/option_groups.dart(1 hunks)lib/src/config/option_resolution.dart(1 hunks)lib/src/config/options.dart(1 hunks)lib/src/config/source_type.dart(1 hunks)pubspec.yaml(1 hunks)test/config/args_compatibility/allow_anything_test.dart(1 hunks)test/config/args_compatibility/args_test.dart(1 hunks)test/config/args_compatibility/command_parse_test.dart(1 hunks)test/config/args_compatibility/command_runner_test.dart(1 hunks)test/config/args_compatibility/command_test.dart(1 hunks)test/config/args_compatibility/parse_performance_test.dart(1 hunks)test/config/args_compatibility/parse_test.dart(1 hunks)test/config/args_compatibility/test_utils.dart(1 hunks)test/config/args_compatibility/trailing_options_test.dart(1 hunks)test/config/args_compatibility/usage_test.dart(1 hunks)test/config/args_compatibility/utils_test.dart(1 hunks)test/config/config_source_test.dart(1 hunks)test/config/configuration_test.dart(1 hunks)test/config/configuration_type_test.dart(1 hunks)test/config/date_parsing_test.dart(1 hunks)test/config/duration_parsing_test.dart(1 hunks)test/config/file_options_test.dart(1 hunks)
🔇 Additional comments (53)
analysis_options.yaml (1)
1-1: Validate base lint preset inclusion
Ensure thatpackage:serverpod_lints/cli.yamlis intentionally used as the root lint preset for your CLI tools. Confirm that any overrides defined below (like disablingunnecessary_final) are being applied on top of this preset in the correct order.lib/config.dart (1)
1-1: Appropriate export approach for modular library design.This single export directive provides a clean entry point for users to access the entire configuration API from one import statement. This approach aligns with Dart package architecture best practices by hiding implementation details while exposing a unified public interface.
pubspec.yaml (3)
18-18: Updated args package dependency.The args package version has been upgraded to ^2.7.0, which is appropriate for ensuring compatibility with the latest features and bug fixes.
20-20: New dependencies support comprehensive configuration functionality.The added dependencies provide key capabilities for the new configuration system:
- collection: Provides advanced data structure utilities
- meta: Offers annotations for improved static analysis
- rfc_6901: Enables JSON Pointer support for navigating configuration objects
- yaml_codec: Allows parsing YAML configuration files
These dependencies are appropriate and necessary for implementing a robust configuration system.
Also applies to: 22-22, 26-26, 28-28
33-33: Development dependency for testing file system operations.The test_descriptor package is an appropriate addition for testing file and directory option validations, which is essential for a configuration system that interacts with the file system.
lib/src/config/source_type.dart (1)
1-10: Well-designed enum for tracking configuration value sources.The
ValueSourceTypeenum provides a clear categorization of where configuration values originate, which is essential for:
- Debugging configuration resolution
- Implementing precedence rules between different sources
- Providing transparency to users about which configuration source took effect
The enum covers all expected configuration sources and is ordered in a way that could potentially reflect precedence (from least to most specific/prioritized sources).
lib/src/config/config.dart (1)
1-10: Well-organized exports for modular configuration system.This central export file follows the best practice of breaking down complex functionality into separate modules while providing a unified API. The exports cover all aspects needed for a comprehensive configuration system:
- Parsing (config_parser, configuration_parser)
- Source management (config_source, config_source_provider, multi_config_source)
- Option definitions and validation (options, file_system_options, option_groups)
- Value provenance tracking (source_type)
- Core configuration handling (configuration)
This approach enhances maintainability while simplifying the import experience for consumers.
test/config/date_parsing_test.dart (4)
1-4: Good test setup with proper imports.The test properly imports the necessary testing framework and the package under test using the public API (
cli_tools/config.dart), which validates that the DateTimeParser is correctly exposed through the public API.
6-17: Appropriate error handling test for empty input.This test verifies that the parser correctly throws a FormatException when given an empty string, which is important for providing clear error messages to users when configuration values are missing or invalid.
18-106: Comprehensive tests for various date-time formats.The test suite provides excellent coverage of different date-time formats:
- ISO dates (2020-01-01)
- Compact dates (20200101)
- Various date-time separators (space, T, t, -, :, _, /)
- UTC indicators (Z, z)
- Fractional seconds
This flexibility is valuable for a configuration system that needs to parse user-provided date values from various sources.
108-116: Good error handling test for invalid format.This test verifies that the parser correctly throws a FormatException for an invalid date-time separator ('x'), ensuring robust error handling for malformed input.
lib/src/config/option_groups.dart (1)
13-21: LGTM! Well-structured option grouping classThe
MutuallyExclusiveclass provides a clean implementation for validating option groups where only one option can be specified. The configurable parameters for mandatory groups and handling of defaults provide good flexibility.test/config/args_compatibility/allow_anything_test.dart (1)
12-13:✅ Verification successful
Verify test_utils.dart contains required utilities
The test imports
test_utils.dart, but it's unclear what utilities are used from this file. Make sure this file exists and contains the necessary helper functions or classes.
🏁 Script executed:
#!/bin/bash # Check if test_utils.dart exists and examine its contents if [ -f "test/config/args_compatibility/test_utils.dart" ]; then echo "File exists. Contents:" cat "test/config/args_compatibility/test_utils.dart" else echo "test_utils.dart not found in expected location." # Try to find it elsewhere find test -name "test_utils.dart" -type f fiLength of output: 7072
No action needed:
test_utils.dartprovides the required utilities
The filetest/config/args_compatibility/test_utils.dartexists and defines all the helper functions and classes used by the tests (e.g., exception matchers and command stubs).test/config/duration_parsing_test.dart (1)
1-244: Well-designed and comprehensive test suite for DurationParserThis is a thorough test suite that covers all the essential aspects of the DurationParser class:
- Error cases with invalid inputs (empty strings, lone hyphens, unit-only strings)
- Parsing of all supported duration units (seconds, minutes, hours, days, milliseconds, microseconds)
- Handling of negative durations
- Normalization of durations (e.g., 70s → 1m10s)
- Formatting durations back to strings
- Compound durations
The test naming follows a clear Given-When-Then pattern, making the test intentions very readable.
test/config/args_compatibility/command_test.dart (2)
1-2: Skip annotation correctly identifies unsupported functionalityGood use of the
@Skipannotation to clearly indicate that CommandRunner functionality is not supported, which aligns with the PR objective of creating compatible config classes.
13-161: Comprehensive command class testing suite for future compatibilityThis test suite thoroughly verifies command behaviors including:
- Default invocation string generation
- Usage string formatting with various options
- Hidden subcommand behavior
- Alias handling
- Line wrapping for long descriptions
- Error message formatting
These tests will be valuable as reference for ensuring the new ConfigParser maintains compatible behavior with ArgParser where applicable.
test/config/args_compatibility/command_parse_test.dart (2)
1-2: Skip annotation correctly identifies unsupported functionalityGood use of the
@Skipannotation to clearly indicate that theaddCommand()functionality in ArgParser is not supported, which aligns with the PR objective of creating compatible config classes.
13-206: Thorough test suite for command parsing behaviorThis test suite provides comprehensive validation of command parsing behavior including:
- Command parser creation and usage
- Error handling for duplicate commands
- Option parsing before and after commands
- Flag and abbreviation handling
- Option assignment to the correct command scope
- Handling of remaining arguments
These tests serve as a valuable reference for understanding the original behavior even if not all functionality is implemented in the new ConfigParser.
test/config/args_compatibility/utils_test.dart (2)
5-6: Direct import from package implementation detailsThe import from
package:args/src/utils.dartrelies on implementation details rather than public API. While this is acceptable for testing, it creates a dependency on the internal implementation of the args package.Consider whether these utility functions should be extracted to your own implementation if they're critical to your package's functionality.
20-216: Comprehensive utility function test suiteThis test suite thoroughly validates the text wrapping and padding utility functions, covering:
- Basic padding functionality
- Text wrapping with various line lengths
- Preservation of indentation
- Handling of newlines
- Minimum wrap length enforcement
- Trailing whitespace handling
- Hanging indent functionality
The tests are well-structured and cover a wide range of edge cases, ensuring robust behavior of these utility functions.
test/config/args_compatibility/parse_performance_test.dart (1)
62-70: The performance assertion is well-structured and informative.The expect statement provides a clear performance guarantee and includes a detailed reason message explaining the assertion. This is a good practice for performance tests as it makes failures easier to debug.
test/config/args_compatibility/trailing_options_test.dart (1)
11-15: The test correctly verifies default behavior.This test ensures that the ConfigParser maintains the same default behavior as the original ArgParser, which is important for backward compatibility.
lib/src/config/config_source_provider.dart (1)
5-14: Well-designed abstraction for configuration source providersThe
ConfigSourceProviderabstract class provides a clean interface for lazily obtaining and caching configuration sources. The documentation clearly explains the purpose and behavior expectations.test/config/args_compatibility/parse_test.dart (3)
1-6: Copyright notice and file setup looks correctThe copyright notice is properly included and the file disables deprecated member use warnings appropriately since this is testing compatibility with an older API.
630-631: Skipped test group for mandatory options behaviorThis test group is skipped with the comment "different error types", which suggests incompatibility between expected and actual error types. Consider implementing proper handling for these different error types to maintain backward compatibility.
This skipped test indicates a potential compatibility issue. Can you explain the difference in error types between the original implementation and the new one? Consider addressing this in a follow-up PR if it's a significant compatibility concern.
821-822: Skipped test for command parsingThis test for command parsing is skipped with the comment "commands not supported". If commands are intentionally not supported in the
ConfigParserbut were supported in the originalArgParser, this should be documented clearly in the migration guide or class documentation.Is the lack of command support a temporary limitation or an intentional design decision? If intentional, please document this difference clearly in the class documentation.
test/config/configuration_type_test.dart (12)
5-9: Good test setup with enum definitionThe enum definition is correctly set up for testing the
EnumOptionfunctionality.
11-42: Well-structured tests for EnumOptionThe test group for
EnumOptionverifies both successful parsing and appropriate error handling when an invalid value is provided. The assertions are clear and validate the expected behavior.
44-96: Comprehensive tests for IntOption basic functionalityThe test group thoroughly validates the
IntOptionbehavior with positive values, negative values, and various error conditions. The error message assertions are specific enough to ensure correct feedback is provided to users.
98-147: Good coverage of ranged IntOptionTests for the ranged
IntOptionproperly verify both in-range values and boundary conditions (values below minimum and above maximum). Error messages are checked for accuracy.
149-194: Thorough testing of IntOption with allowedValuesTesting of the allow-list functionality includes verification from both command-line arguments and environment variables, ensuring consistent behavior across different input sources.
196-285: Comprehensive tests for DurationOptionThe tests for
DurationOptioncover all supported time units (days, hours, minutes, seconds) as well as values without units. The range validation tests are thorough and verify appropriate error messages.
287-432: Well-structured tests for MultiOption with string valuesThis test group thoroughly covers the
MultiOptionwith string values, testing parsing from different sources (args, env vars, config) and handling various formats (single values, multiple values, comma-separated values). Type checking and error handling are properly verified.
434-589: Comprehensive tests for MultiOption with integer valuesThe test group for
MultiOptionwith integer values covers all necessary aspects including empty values, parsing errors, and type validation. The tests are well-structured and cover the expected behavior thoroughly.
591-658: Good coverage of MultiOption with default valuesTests for
MultiOptionwith default values properly verify that defaults are applied when no values are provided and are overridden when values are present.
660-742: Thorough tests for mandatory MultiOption with aliasesThis test group verifies that mandatory
MultiOptioninstances correctly report errors when no values are provided and properly handle aliases for argument names.
744-822: Complete testing of MultiStringOption with allowedValuesTests for
MultiStringOptionwith an allow-list properly verify behavior with valid values, empty values, and invalid values, ensuring appropriate error handling.
825-834: Well-implemented test helperThe
_TestConfigBrokerimplementation provides a simple and effective way to test configuration retrieval from broker sources. The implementation is minimal yet sufficient for the testing needs.test/config/config_source_test.dart (5)
5-63: Well-structured tests for MultiDomainConfigBroker with YAML contentThe first test group properly sets up options for both YAML and JSON content and verifies that the YAML content is correctly parsed and values are retrieved from the appropriate domain. The test structure is clear and the assertions validate the expected behavior.
65-86: Good test coverage for JSON content parsingThis test effectively verifies that JSON content is properly parsed and values are correctly retrieved from the appropriate domain. The test confirms that domains are properly isolated with the JSON domain returning values and the YAML domain returning null.
88-147: Thorough error testing for type mismatchesThese tests verify that proper error handling occurs when configuration values have the wrong type (int instead of string) in both YAML and JSON content. The error messages are specific and helpful, and the tests confirm that accessing invalid values throws the expected exception.
149-204: Good error handling for malformed contentThese tests verify that malformed YAML and JSON content is properly detected and appropriate error messages are generated. The error messages contain helpful context information and the tests confirm that accessing invalid values throws the expected exception.
207-258: Effective testing of misconfigured optionsThis test group verifies that appropriate errors are thrown when option configuration keys don't match any available domain. The test checks both missing domain prefixes and unknown domain prefixes, ensuring comprehensive validation.
test/config/args_compatibility/usage_test.dart (1)
1-548: Well-structured comprehensive test suite for ConfigParser.usageThis test file provides excellent coverage of ConfigParser usage string generation functionality, covering various scenarios including flag behavior, option alignment, help text formatting, default values, allowed values, wrapping behavior, and separators. The validation helpers are well-implemented.
I particularly like the thorough testing of edge cases like text wrapping with different usageLineLength values, empty line handling, and mutual exclusion with error cases.
lib/src/config/file_system_options.dart (4)
7-11: Good enum design with clear intentThe PathExistMode enum provides a clear and concise way to specify path existence constraints for file system operations.
13-20: Simple and effective parser implementationThe DirParser follows the single responsibility principle by focusing solely on converting strings to Directory objects.
77-84: Consistent parser implementationThe FileParser follows the same pattern as DirParser, maintaining consistency in the codebase.
86-139: Parallel implementation with appropriate error handlingThe FileOption class mirrors the DirOption class structure but with file-specific validation logic. This parallel design makes the code more maintainable and predictable.
test/config/file_options_test.dart (2)
7-231: Comprehensive DirOption testingThe test suite thoroughly covers all PathExistMode scenarios for DirOption with clear assertions and good test organization. The test cases verify both success paths and error conditions.
233-456: Consistent FileOption testingThe FileOption tests follow the same structured approach as the DirOption tests, providing parallel validation of functionality. All combinations of path existence modes and path types are covered, ensuring robust validation.
lib/src/config/multi_config_source.dart (2)
4-27: Well-documented configuration brokerThe class documentation clearly explains the purpose and behavior of the MultiDomainConfigBroker with good examples and explanations of the regex pattern matching behavior.
28-63: Flexible domain matching with multiple constructor optionsThe implementation provides three constructor variants that allow different ways to specify domain matching patterns, making the class adaptable to various configuration scenarios.
test/config/args_compatibility/test_utils.dart (1)
342-349: Helper enforcesFormatException, butArgParser.parse()now throwsArgParserException
throwsFormat()expects a plainFormatException, yet theargs
package switched to throwing anArgParserException(which wraps the
format error) in most failure cases.
Relying on the older behaviour may cause brittle tests when the
dependency is upgraded. You can future‑proof by catching both:void throwsFormat(ArgParser parser, List<String> args, {String? reason}) { expect( () => parser.parse(args), throwsA(anyOf(isA<FormatException>(), isA<ArgParserException>())), reason: reason, ); }
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/src/config/configuration.dart (3)
337-346:⚠️ Potential issueUse standard index operator instead of non-existent option() method
The base
_resolveNamedArgmethod usesargs.option(argOptName)which is not part of the standardArgResultsAPI. Use the index operator instead.OptionResolution<V>? _resolveNamedArg(final ArgResults? args) { final argOptName = argName; if (argOptName == null || args == null || !args.wasParsed(argOptName)) { return null; } return OptionResolution( - stringValue: args.option(argOptName), + stringValue: args[argOptName] as String?, source: ValueSourceType.arg, ); }
500-510:⚠️ Potential issueUse standard index operator instead of non-existent flag() method
The
_resolveNamedArgmethod inFlagOptionusesargs.flag(argOptName)which is not part of the standardArgResultsAPI. Use the index operator instead.@override OptionResolution<bool>? _resolveNamedArg(final ArgResults? args) { final argOptName = argName; if (argOptName == null || args == null || !args.wasParsed(argOptName)) { return null; } return OptionResolution( - value: args.flag(argOptName), + value: args[argOptName] as bool, source: ValueSourceType.arg, ); }
316-324:⚠️ Potential issueFix positional index consumption when using named arguments
When an option is defined with both
argNameandargPos, and the named form is used, you must still advance the positional arguments iterator to keep subsequent positional arguments aligned correctly.result = _resolveNamedArg(args); - if (result != null) return result; + if (result != null) { + // Consume the positional slot if this option also has argPos + if (argPos != null && posArgs != null) { + posArgs.moveNext(); // discard value + } + return result; + }
🧹 Nitpick comments (5)
lib/src/config/config_parser.dart (2)
243-261: Consider adding error handling for callback invocationsWhen invoking callbacks in
_invokeCallbacks, consider catching and logging exceptions to prevent a single callback failure from stopping subsequent callbacks from running.void _invokeCallbacks(final Configuration cfg) { for (final entry in _flagCallbacks.entries) { + try { entry.value(cfg.findValueOf<bool>(argName: entry.key) ?? false); + } catch (e) { + // Log or handle the callback error + } } for (final entry in _optionCallbacks.entries) { + try { entry.value(cfg.findValueOf<String>(argName: entry.key)); + } catch (e) { + // Log or handle the callback error + } } for (final entry in _multiOptionCallbacks.entries) { + try { entry.value(cfg.findValueOf<List<String>>(argName: entry.key) ?? []); + } catch (e) { + // Log or handle the callback error + } } }
55-56: Consider adding methods for managing option groupsThe current implementation doesn't expose methods for adding or managing option groups directly through
ConfigParser. Consider adding methods to simplify working with option groups./// Adds a separator to the usage output. OptionGroup addGroup(String name) { final group = OptionGroup(name); return group; } /// Creates a mutually exclusive option group. OptionGroup addMutuallyExclusiveGroup(String name) { // Implementation would depend on your OptionGroup implementation // that supports mutual exclusion }lib/src/config/options.dart (3)
332-345: Consider adding compact formatting option for durationsThe current
formatmethod always produces the most verbose output for durations. Consider adding a parameter to control whether to use a compact format (showing only the largest non-zero unit) or the full format.@override - String format(final Duration value) { + String format(final Duration value, {bool compact = false}) { if (value == Duration.zero) return '0s'; final sign = value.isNegative ? '-' : ''; final d = _unitStr(value.inDays, null, 'd'); + + if (compact && d.isNotEmpty) return '$sign$d'; final h = _unitStr(value.inHours, 24, 'h'); + if (compact && h.isNotEmpty) return '$sign$h'; final m = _unitStr(value.inMinutes, 60, 'm'); + if (compact && m.isNotEmpty) return '$sign$m'; final s = _unitStr(value.inSeconds, 60, 's'); + if (compact && s.isNotEmpty) return '$sign$s'; final ms = _unitStr(value.inMilliseconds, 1000, 'ms'); + if (compact && ms.isNotEmpty) return '$sign$ms'; final us = _unitStr(value.inMicroseconds, 1000, 'us'); return '$sign$d$h$m$s$ms$us'; }
234-254: Consider adding more flexible date formats to DateTimeParserWhile the DateTimeParser is more forgiving than DateTime.parse by supporting multiple separators, consider adding support for more date-time formats such as ISO 8601 variants and common formats like "MM/DD/YYYY".
@override DateTime parse(final String value) { final val = DateTime.tryParse(value); if (val != null) return val; if (value.length >= 11 && '-_/:t'.contains(value[10])) { final val = DateTime.tryParse('${value.substring(0, 10)}T${value.substring(11)}'); if (val != null) return val; } + // Try common formats like MM/DD/YYYY + final dateFormats = [ + RegExp(r'^(\d{1,2})/(\d{1,2})/(\d{4})(?:\s+(\d{1,2}):(\d{1,2})(?::(\d{1,2}))?)?$'), + // Add more format patterns as needed + ]; + for (final format in dateFormats) { + final match = format.firstMatch(value); + if (match != null) { + // Extract and parse components from match + // Return parsed DateTime + } + } throw FormatException('Invalid date-time "$value"'); }
90-110: Consider making EnumParser case-insensitiveThe
EnumParsercurrently requires an exact, case-sensitive match for enum values. For command-line tools, it's often more user-friendly to support case-insensitive matching.class EnumParser<E extends Enum> extends ValueParser<E> { final List<E> enumValues; + final bool caseSensitive; - const EnumParser(this.enumValues); + const EnumParser(this.enumValues, {this.caseSensitive = false}); @override E parse(final String value) { + if (caseSensitive) { return enumValues.firstWhere( (final e) => e.name == value, orElse: () => throw FormatException( '"$value" is not in ${valueHelpString()}', ), ); + } else { + return enumValues.firstWhere( + (final e) => e.name.toLowerCase() == value.toLowerCase(), + orElse: () => throw FormatException( + '"$value" is not in ${valueHelpString()}', + ), + ); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/config/config_parser.dart(1 hunks)lib/src/config/configuration.dart(1 hunks)lib/src/config/options.dart(1 hunks)test/config/duration_parsing_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/config/duration_parsing_test.dart
🔇 Additional comments (8)
lib/src/config/config_parser.dart (4)
389-392: Good use ofwhereType<String>()instead of.nonNullsYour implementation correctly uses
whereType<String>()to filter the collection for String elements, addressing a previous issue where.nonNullsextension was being used without importing the collection package.
226-234: Configuration broker is now properly forwardedI see you've correctly forwarded the
configBrokerparameter toConfiguration.resolve(), fixing a previous issue where the parameter was accepted but not used.
8-23: Clear documentation of compatibility and purposeThe class documentation clearly explains that this is a backward-compatible replacement for
ArgParserwith almost complete compatibility except for subcommands. It also properly explains the purpose of enabling easy transition to the configuration package.
304-335: Clear documentation of ConfigResults compatibilityThe class documentation for
ConfigResultsclearly explains its purpose as a backward-compatible wrapper aroundConfigurationthat implements theArgResultsinterface, with noted exceptions for unsupported features.lib/src/config/options.dart (2)
337-338: Fixed DurationParser.format() day handlingThe implementation now correctly handles days by using
nullas the mod parameter in_unitStr, which ensures that whole-day multiples are not dropped. This fixes the issue whereDuration(days: 30)would be formatted as6dinstead of30d.
283-298: Comprehensive DurationParser implementationThe DurationParser implementation is well-documented and handles a variety of formats with clear error messages. The support for different time units (s, m, h, d, ms, us) makes it flexible for different use cases.
lib/src/config/configuration.dart (2)
446-454: BoolParser now correctly throws on invalid inputsThe BoolParser implementation now correctly uses
bool.parse()instead of the previousbool.tryParse() ?? false, ensuring that invalid inputs will throw exceptions rather than silently returning false.
93-100: Ensure documentation matches implementationThe documentation states that when an option is used as both named and positional, the positional index should still be consumed. However, the implementation doesn't reflect this behavior. After fixing the implementation, ensure that you add tests to verify this behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/config/args_compatibility/command_runner_test.dart (1)
308-319: Fix typos in test descriptionsTiny spelling issues won’t break execution, but they do reduce polish and make searching harder.
- test('runs a hidden comand', () { + test('runs a hidden command', () { ... - test('runs an aliased comand', () { + test('runs an aliased command', () {test/config/args_compatibility/args_test.dart (1)
7-8: Remove unused import to silence analyzer warnings
package:args/args.dartis no longer referenced in this file.
Keeping it triggersunused_importhints (unless globally disabled).-import 'package:args/args.dart';If you added it to get access to internal types (e.g.
Option), note that they’re not publicly exported byargs; the newcli_toolsexport already covers what’s needed.test/config/args_compatibility/parse_test.dart (1)
597-630: Confirm skipped “mandatory” tests still give coverageThis entire group is skipped with the note
different error types.
While that’s fine during migration, it leaves a gap in verifying mandatory-option semantics forConfigParser.Consider:
• Re-enabling the tests once the exception surface stabilises, or
• Adding new expectations that match the current behaviour so that mandatory option handling remains guarded by CI.test/config/configuration_test.dart (1)
176-185: Consider clarifying the behavior for duplicate arguments.The test comments note that accepting the last value for duplicate arguments is the behavior of ArgParser, but suggests it might be worth considering making this a usage error instead.
Consider adding a TODO comment or creating an issue to revisit this behavior later, especially since this might be unexpected for users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
analysis_options.yaml(1 hunks)lib/config.dart(1 hunks)lib/src/config/config.dart(1 hunks)lib/src/config/config_parser.dart(1 hunks)lib/src/config/config_source.dart(1 hunks)lib/src/config/config_source_provider.dart(1 hunks)lib/src/config/configuration.dart(1 hunks)lib/src/config/configuration_parser.dart(1 hunks)lib/src/config/file_system_options.dart(1 hunks)lib/src/config/json_yaml_document.dart(1 hunks)lib/src/config/multi_config_source.dart(1 hunks)lib/src/config/option_groups.dart(1 hunks)lib/src/config/option_resolution.dart(1 hunks)lib/src/config/options.dart(1 hunks)lib/src/config/source_type.dart(1 hunks)pubspec.yaml(1 hunks)test/config/args_compatibility/allow_anything_test.dart(1 hunks)test/config/args_compatibility/args_test.dart(1 hunks)test/config/args_compatibility/command_parse_test.dart(1 hunks)test/config/args_compatibility/command_runner_test.dart(1 hunks)test/config/args_compatibility/command_test.dart(1 hunks)test/config/args_compatibility/parse_performance_test.dart(1 hunks)test/config/args_compatibility/parse_test.dart(1 hunks)test/config/args_compatibility/test_utils.dart(1 hunks)test/config/args_compatibility/trailing_options_test.dart(1 hunks)test/config/args_compatibility/usage_test.dart(1 hunks)test/config/args_compatibility/utils_test.dart(1 hunks)test/config/config_source_test.dart(1 hunks)test/config/configuration_test.dart(1 hunks)test/config/configuration_type_test.dart(1 hunks)test/config/date_parsing_test.dart(1 hunks)test/config/duration_parsing_test.dart(1 hunks)test/config/file_options_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- analysis_options.yaml
- test/config/args_compatibility/command_test.dart
- test/config/args_compatibility/allow_anything_test.dart
- test/config/args_compatibility/test_utils.dart
🚧 Files skipped from review as they are similar to previous changes (25)
- lib/config.dart
- lib/src/config/option_groups.dart
- test/config/date_parsing_test.dart
- lib/src/config/source_type.dart
- test/config/args_compatibility/utils_test.dart
- lib/src/config/config.dart
- test/config/args_compatibility/command_parse_test.dart
- pubspec.yaml
- lib/src/config/json_yaml_document.dart
- test/config/duration_parsing_test.dart
- test/config/configuration_type_test.dart
- lib/src/config/configuration_parser.dart
- test/config/args_compatibility/trailing_options_test.dart
- lib/src/config/multi_config_source.dart
- lib/src/config/config_source_provider.dart
- lib/src/config/config_source.dart
- test/config/config_source_test.dart
- test/config/args_compatibility/usage_test.dart
- test/config/args_compatibility/parse_performance_test.dart
- lib/src/config/file_system_options.dart
- test/config/file_options_test.dart
- lib/src/config/option_resolution.dart
- lib/src/config/config_parser.dart
- lib/src/config/configuration.dart
- lib/src/config/options.dart
🔇 Additional comments (15)
test/config/configuration_test.dart (15)
1-5: Well-structured imports and organization.The file has appropriate imports for testing the configuration framework, importing both the test utilities and the package under test.
7-24: Good validation testing for invalid configurations.This test correctly validates that options with abbreviations must have a full name. The test properly checks that an
InvalidOptionConfigurationErroris thrown with the appropriate error message.
26-67: Thorough validation tests for mandatory options with defaults.These tests properly verify that mandatory options cannot have default values, either as direct constants or from functions. This ensures proper configuration validation and prevents contradictory option definitions.
69-113: Comprehensive tests for basic option parsing.This test group thoroughly covers the basic functionality of adding and parsing options, including:
- Adding options to the parser
- Parsing values from command line
- Detecting when options are parsed
- Handling misspelled options
- Handling duplicate options
Good coverage of core functionality.
115-186: Well-structured precedence tests.This test group verifies the precedence order of configuration sources:
- Command line arguments (highest)
- Environment variables
- Configuration sources
- Custom value functions
- Default values (lowest)
The tests are clear and each precedence rule is tested independently.
314-338: Verify optional value behavior consistency.The tests show that
value()throws aStateErrorfor optional values, whileoptionalValue()returns null. This design choice forces users to be explicit about which method to use based on whether they expect a value to be present.Ensure this behavior is clearly documented in the public API docs so users understand when to use each method.
477-533: Comprehensive validation of invalid option combinations.These tests effectively validate that the library prevents problematic option configurations:
- Duplicate argument names
- Duplicate environment variable names
- Duplicate argument positions
- Non-consecutive argument positions
- Positional arguments that don't start at 0
This helps ensure robust configuration validation.
869-949: Good alias support tests.The tests thoroughly verify that options can be provided via aliases, and that the aliases are properly resolved to the same option when parsing. This provides good flexibility for command-line interfaces.
951-1213: Thorough mutually exclusive option tests.These tests cover:
- Basic mutually exclusive groups
- Mandatory mutually exclusive groups (requiring one option from the group)
- Multiple mutually exclusive groups
The tests verify both the success cases and error cases, ensuring that the mutual exclusion constraints are properly enforced.
1215-1262: Good testing of dependency ordering.This test verifies that configuration options can depend on other options, but they must be processed in the correct order. The test correctly validates that a configuration broker can't depend on options that haven't been resolved yet.
1267-1304: Type safety verification for direct value construction.These tests verify that the
Configuration.fromValuesconstructor enforces type safety, which is important for preventing runtime errors when directly constructing configurations.
1327-1440: Comprehensive enum-based option definition tests.These tests verify that option values can be accessed through various identifiers (enum name, argument name, position, environment variable name, config key), providing flexibility in how options are referenced.
1443-1465: Well-implemented test enum for option definitions.The
_TestOptionenum is a good implementation of theOptionDefinitioninterface, demonstrating how options can be defined as enum values for better type safety and organization.
1467-1485: Good testing utilities for configuration brokers.The
_TestConfigBrokerclass and_dependentConfigBrokerfunction provide useful utilities for testing configuration brokers, including dependencies between configuration options.Also applies to: 1487-1494
1496-1512: Well-designed helper functions for value resolution.These helper functions provide mock implementations for testing default values and custom value resolution, which is important for testing the configuration precedence rules.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
lib/src/better_command_runner/config_resolver.dart (2)
40-44: Handle surrounding whitespace when formatting config errors
formatConfigError()capitalises the first character without trimming leading spaces, so" missing file"becomes" Missing file". Consider trimming the message first to avoid such artefacts and to ensure the initial punctuation test runs on the actual first character.- if (error.isEmpty) return error; - final suffix = _isPunctuation(error.substring(error.length - 1)) ? '' : '.'; - return '${error[0].toUpperCase()}${error.substring(1)}$suffix'; + final trimmed = error.trimLeft(); + if (trimmed.isEmpty) return trimmed; + final suffix = + _isPunctuation(trimmed.substring(trimmed.length - 1)) ? '' : '.'; + return '${trimmed[0].toUpperCase()}${trimmed.substring(1)}$suffix';This keeps log output tidy and avoids double spaces in multi-line error blocks.
47-48: Micro-optimisation: avoid compilingRegExpon every call
_isPunctuationcreates a newRegExpeach invocation. Caching it as aconstsaves unnecessary allocations without impacting readability.-bool _isPunctuation(final String char) { - return RegExp(r'\p{P}', unicode: true).hasMatch(char); -} +const _punctuationRe = RegExp(r'\p{P}', unicode: true); + +bool _isPunctuation(final String char) => _punctuationRe.hasMatch(char);lib/src/better_command_runner/better_command.dart (1)
25-29: Constructor forwardswrapTextColumnbut never exposes it
wrapTextColumnis forwarded to the internalArgParser, yet there is no way for callers to read it back or override it later. This is fine, but consider documenting the parameter (and default) in the public API to make the intent clear—especially because the previousArgParser()default differs between SDK versions.lib/src/better_command_runner/better_command_runner.dart (3)
60-63: Remove leftoverfooconstant – it is unused and confusing
foodoes not appear anywhere else in the class and its name conveys no meaning. Leaving dead constants around increases cognitive load and may be mistaken for an intended public API later.- static const foo = <OptionDefinition>[ - BetterCommandRunnerFlags.verboseOption, - BetterCommandRunnerFlags.quietOption, - ];
74-75: Fix typo in documentationThe word “gloabl” should be “global”.
- /// The gloabl option definitions. + /// The global option definitions.
273-279: Consider making log-level precedence explicitCurrent logic puts
--verboseabove--quiet, but if both are supplied the configuration still stores both flags astrue, which could confuse downstream consumers.You could normalise the configuration once:
static CommandRunnerLogLevel _determineLogLevel(Configuration config) { - if (config.findValueOf(argName: BetterCommandRunnerFlags.verbose) == true) { + final verbose = config.findValueOf(argName: BetterCommandRunnerFlags.verbose) == true; + final quiet = config.findValueOf(argName: BetterCommandRunnerFlags.quiet) == true; + + if (verbose && quiet) { + // verbose wins – optionally emit a warning. + return CommandRunnerLogLevel.verbose; + } + if (verbose) { return CommandRunnerLogLevel.verbose; - } else if (config.findValueOf(argName: BetterCommandRunnerFlags.quiet) == - true) { + } + if (quiet) { return CommandRunnerLogLevel.quiet; } return CommandRunnerLogLevel.normal; }This makes the precedence rule self-documenting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/better_command_runner/better_command.dart(2 hunks)lib/src/better_command_runner/better_command_runner.dart(7 hunks)lib/src/better_command_runner/config_resolver.dart(1 hunks)test/better_command_test.dart(2 hunks)
🔇 Additional comments (2)
lib/src/better_command_runner/better_command.dart (1)
46-50:run()blindly returns whateverrunWithConfigyields – verify nullability
run()always returns the result ofrunWithConfig, butrunWithConfigcurrently allows returningnull. Down-stream code (and tests) may assume a non-nullFuture<T>; enforcing a non-nullable return here would prevent surprises:- FutureOr<T>? run() { + FutureOr<T> run() { final config = resolveConfiguration(argResults); return runWithConfig(config); }(and adjust
runWithConfigaccordingly).lib/src/better_command_runner/better_command_runner.dart (1)
190-199: Re-entrancy & concurrency: confirm_globalConfigurationsafety
_globalConfigurationis reassigned every timerunCommandis invoked.
BecauserunCommandreturns aFuture, nothing prevents a second invocation from starting before the first finishes (e.g. if a wrapper calls it in parallel). In that case the field would be overwritten mid-execution.If parallel invocations are possible, convert the field to a local variable and pass it through the call chain, or protect the critical section with a lock.
Please verify the intended usage pattern.
SandPod
left a comment
SandPod
left a comment
There was a problem hiding this comment.
Super nice additions!
I only inspected the tests to get an understanding of what features this introduces.
This is a great expansion of the toolset with nice tests!
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
test/better_command_test.dart (1)
100-101:⚠️ Potential issueReplace arbitrary delays with deterministic signaling
Using
Future.delayedwith fixed durations makes tests flaky:
- CI environments may have different timing characteristics
- The delays slow down the test suite
- Race conditions can still occur
This was previously flagged in a code review and should be addressed.
Use a
Completerin your analytics callback to signal when analytics events have been processed:final completer = Completer<void>(); var analyticsEvents = <String>[]; var runner = BetterCommandRunner( 'test', 'test project', onAnalyticsEvent: (e) { analyticsEvents.add(e); completer.complete(); // signal test }, messageOutput: messageOutput, ); // ... await runner.run(['--help']); await completer.future; // deterministic wait instead of arbitrary delayAlso applies to: 110-111, 120-121, 130-131, 141-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/better_command_runner/better_command.dart(2 hunks)lib/src/better_command_runner/config_resolver.dart(1 hunks)test/better_command_test.dart(2 hunks)test/documentation_generator/generate_markdown_test.dart(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/src/better_command_runner/config_resolver.dart
🔇 Additional comments (9)
test/documentation_generator/generate_markdown_test.dart (2)
1-2: Added import for async operations is appropriate.The
dart:asyncimport is necessary for theFutureOrreturn type used in the newly addedrunWithConfigmethods.
5-5: Import for Configuration class is correctly added.This import provides access to the
Configurationclass from the new config library, which is necessary for the implementation of therunWithConfigmethod.lib/src/better_command_runner/better_command.dart (5)
10-10: Type parameters correctly defined for BetterCommand class.Good work making
BetterCommandgeneric withO extends OptionDefinitionandT. This provides strong typing for both option definitions and the command's return type.
20-29: Constructor properly handles initialization with options and resolver.The constructor is well-designed:
- Accepts optional message output and wrap column
- Takes a list of option definitions with a sensible default empty list
- Allows injection of a custom config resolver with a good default
This provides flexibility while maintaining ease of use.
39-50: Effective separation of concerns in run method.The
run()method now correctly:
- Resolves configuration from various sources
- Delegates actual command execution to
runWithConfig()This separation allows subclasses to focus on implementing their logic with a fully resolved configuration.
59-73: Good error handling in resolveConfiguration.The configuration resolution process:
- Uses the injected resolver to process options and arguments
- Checks for errors and formats them appropriately
- Throws a usage exception with clear error messages when needed
This ensures users get helpful feedback when their configuration is invalid.
77-77: Abstract method declaration is appropriate.Making
runWithConfigabstract enforces that subclasses must implement it, which is better than throwing a runtime exception. This was a good improvement from the previous code where there was a non-abstract method throwingUnimplementedError.test/better_command_test.dart (2)
7-17: Well-structured enum for global options.The
BespokeGlobalOptionenum is a good implementation ofOptionDefinition:
- Properly implements the required interface
- Provides a variety of option types for testing
- Includes validation constraints on the age option
This will effectively test different aspects of the configuration system.
22-29: MockCommand now uses options constructor properly.The command correctly uses the new options-based constructor pattern with a well-defined string option, including default values and allowed values constraints.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/better_command_test.dart (2)
40-43: Fix mock implementation of runWithConfigThe
runWithConfigmethod inMockCommandis throwingUnimplementedError, which will cause tests to fail when this method is called. This issue was already identified in a previous review.@override FutureOr? runWithConfig(Configuration<OptionDefinition> commandConfig) { - throw UnimplementedError(); + // Simplified implementation for testing + return null; }
95-143:Future.delayedmakes tests flaky – use explicit signalling insteadMultiple test cases are using
await Future.delayed(...)to wait for analytics events to be processed. This approach is problematic because:
- It makes tests unreliable and flaky, especially in CI environments
- It unnecessarily slows down the test suite
- Race conditions can still occur if the delay isn't long enough
A better approach would be to use explicit signaling with a Completer:
// In test setup final completer = Completer<void>(); var analyticsEvents = <String>[]; var runner = BetterCommandRunner( 'test', 'test project', onAnalyticsEvent: (e) { analyticsEvents.add(e); completer.complete(); // signal test when analytics is processed }, messageOutput: messageOutput, ); // In test await runner.run(['--help']); await completer.future; // wait for analytics to be processed expect(analyticsEvents, hasLength(1));
🧹 Nitpick comments (1)
test/better_command_test.dart (1)
1-348: Consider organizing tests with more granulardescribe/testblocksWhile the current organization separates tests into logical groups based on configuration, consider further structuring within groups to improve readability and maintenance:
- Group related assertions together (e.g., help text verification vs. analytics events)
- Extract common test setup into setup functions
- Use more descriptive test names that follow the "Given/When/Then" pattern consistently
This restructuring would make the test file more maintainable as it grows and make failures easier to diagnose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/better_command_test.dart(2 hunks)
🔇 Additional comments (2)
test/better_command_test.dart (2)
7-17: LGTM: Well-defined option enum with validation constraintsThe
BespokeGlobalOptionenum is a good implementation of theOptionDefinitioninterface for testing purposes. It correctly defines global options with appropriate constraints, including an age option with min/max validation.
246-347: Comprehensive test coverage for global optionsThe tests in this group thoroughly verify that custom global options (including the
ageoption with validation constraints) are properly integrated and displayed in help text. This ensures the new configuration system works correctly with both default and custom options.
Migrated the
configlibrary as used in the scloud CLI tool to thecli_toolspackage.Added
ConfigParserandConfigResultsas wrappers aroundConfigurationthat are drop-in replacements forArgParserandArgResults, to help an easy transition to use theconfiglibrary.Copied the args package test suite and applied it to ConfigParser to ensure backwards compatibility.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores