refactor: Simplified default usage of BetterCommand/Runner #41
Conversation
📝 WalkthroughWalkthroughThe changes introduce a refactored CLI structure with enhanced logging and configuration resolution. The main function now uses a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant main.dart
participant BetterCommandRunner
participant TimeSeriesCommand
participant Logger
User->>main.dart: Run CLI with arguments
main.dart->>BetterCommandRunner: Initialize with global options
main.dart->>BetterCommandRunner: run(args)
BetterCommandRunner->>BetterCommandRunner: Parse args, resolve config
BetterCommandRunner->>Logger: Set log level (verbose/quiet/info)
BetterCommandRunner->>TimeSeriesCommand: Execute if "series" command
TimeSeriesCommand->>TimeSeriesCommand: Validate mutually exclusive options
TimeSeriesCommand->>TimeSeriesCommand: Calculate interval or length
TimeSeriesCommand->>main.dart: Output generated timestamps
BetterCommandRunner->>main.dart: Handle UsageException if thrown
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:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
example/main.dart (2)
1-4: Remove unusedargsimport
package:args/command_runner.dartis imported but never referenced. Dead imports increase analysis time and may triggerunused_importlints.-import 'package:args/command_runner.dart';
10-16: Typo in CLI description
'Example CLI commmand'contains three “m”s in “command”.- 'Example CLI commmand', + 'Example CLI command',lib/src/better_command_runner/better_command.dart (1)
65-73: Micro-optimisation: use identical() for compile-time constant comparisonWhen comparing const instances,
identical(a, b)is slightly faster and avoids
deep equality semantics that might change later.- if (_messageOutput != _defaultMessageOutput) { + if (!identical(_messageOutput, _defaultMessageOutput) && + _messageOutput != null) {lib/src/better_command_runner/better_command_runner.dart (1)
204-210:runswallows synchronousUsageExceptionstack tracesWrapping the body in
Future.synchides the original synchronous stack,
making debugging harder. Returningsuper.rundirectly preserves it and is
still fully async becauserunCommandreturns a future.- Future<T?> run(Iterable<String> args) { - return Future.sync(() { - ... - }); - } + @override + Future<T?> run(Iterable<String> args) async { + final argResults = parse(args); + globalConfiguration = resolveConfiguration(argResults); + return await runCommand(argResults); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
example/main.dart(1 hunks)lib/src/better_command_runner/better_command.dart(4 hunks)lib/src/better_command_runner/better_command_runner.dart(8 hunks)lib/src/better_command_runner/config_resolver.dart(2 hunks)lib/src/config/configuration.dart(3 hunks)pubspec.yaml(1 hunks)test/better_command_runner/analytics_test.dart(5 hunks)test/better_command_runner/default_flags_test.dart(1 hunks)test/better_command_runner/logging_test.dart(2 hunks)test/better_command_runner/parse_log_level_test.dart(1 hunks)test/better_command_test.dart(4 hunks)
🔇 Additional comments (16)
pubspec.yaml (1)
22-22:Details
✅ Verification successful
Verify compatibility with older meta package version
The dependency constraint for the
metapackage has been relaxed from^1.16.0to^1.11.0. While this increases compatibility with projects using older versions, it's important to ensure that your code doesn't rely on features introduced in newer versions.
🏁 Script executed:
#!/bin/bash # Check if the codebase uses any features from meta package versions > 1.11.0 # Search for imports of the meta package echo "Searching for meta package imports..." rg "import 'package:meta/.+" -l # Examine usages of meta annotations/features echo "Examining meta package usages..." rg -A 3 -B 3 "@\w+\b" | grep -E "@(sealed|immutable|nonVirtual|optionalTypeArgs|mustBeOverridden|mustCallSuper|protected|visibleForTesting|internal|useResult)"Length of output: 618
🌐 Web query:
Which version of the Dart meta package introduced the @mustCallSuper annotation?💡 Result:
The @mustCallSuper annotation was introduced in version 0.9.0 of the Dart meta package. According to the changelog for meta 0.12.1, it specifically states:
"Introduce @mustCallSuper annotation for methods that must be invoked by all overriding methods." and lists this change under 0.9.0, indicating that was the initial version to add it[8].
Therefore, the @mustCallSuper annotation first appeared in meta version 0.9.0.
Citations:
- 1: dart-lang/sdk#51253
- 2: https://api.flutter.dev/flutter/meta/
- 3: https://pub.dev/packages/meta/changelog
- 4: https://api.flutter.dev/flutter/meta/mustCallSuper-constant.html
- 5: https://dart.dev/language/metadata
- 6: https://github.com/dart-lang/linter/issues/194
- 7: dart-lang/sdk#53682
- 8: https://pub.dev/packages/meta/versions/0.12.1/changelog
- 9: https://pub.dev/packages/meta
- 10: https://api.flutter.dev/flutter/meta/Required-class.html
Meta package compatibility confirmed
Only the
@mustCallSuperannotation is used (inlib/src/config/configuration.dartandlib/src/config/options.dart), which was introduced in meta 0.9.0. Since your constraint is now^1.11.0, all used annotations are supported. No further action is needed.test/better_command_runner/parse_log_level_test.dart (1)
30-30: Added required messageOutput parameterThe
messageOutputparameter has been added to theBetterCommandRunnerconstructor, which is consistent with the PR objective of making terminal output behavior more consistent. This addition ensures that test setup aligns with the updated constructor signature.test/better_command_test.dart (3)
53-53: Renamed parameter for improved clarityThe parameter name has been changed from
logUsagetousageLoggerfor theMessageOutputconstructor, which improves naming consistency and makes the parameter's purpose clearer.
152-152: Renamed parameter for improved clarityThe parameter name has been changed from
logUsagetousageLoggerfor theMessageOutputconstructor, maintaining consistency with the earlier instance.
202-202: Renamed parameter for improved clarityThe parameter name has been changed from
logUsagetousageLoggerfor theMessageOutputconstructor, maintaining consistency throughout the test file.test/better_command_runner/default_flags_test.dart (1)
10-10: Added required messageOutput parameterThe
messageOutputparameter has been added to theBetterCommandRunnerconstructor, which aligns with the PR objective of making terminal output behavior more consistent. This change synchronizes the test setup with the updated constructor signature and enables the standardized handling of command output and logging across the codebase.test/better_command_runner/analytics_test.dart (1)
62-62: Added messageOutput parameter to all BetterCommandRunner instancesThis change adds the required
messageOutputparameter to all instances ofBetterCommandRunnerin the test suite, ensuring consistent usage message and exception handling. This aligns with the refactoring in the PR that makes output behavior consistent with the standardCommandandCommandRunnerclasses.Also applies to: 82-82, 101-101, 214-214, 279-279
test/better_command_runner/logging_test.dart (2)
37-38: Updated parameter names to more descriptive versionsThe parameter names have been updated from
logUsageExceptiontousageExceptionLoggerand fromlogUsagetousageLogger, which better describes their purpose as callback functions. This improves code readability and better indicates the role of these parameters.
77-77: Updated expected error message formatThe error message for invalid commands has been updated to match the standard format from the args package: "Could not find a command named...". This provides a more consistent user experience between
BetterCommandRunnerand the standardCommandRunner.lib/src/config/configuration.dart (3)
9-12: Improved documentation for OptionDefinition interfaceThe documentation for
OptionDefinition<V>has been enhanced to clarify the role of the type parameter[V]. This provides better guidance for developers implementing this interface, aligning with the PR's goal of improving the developer experience.
58-59: Enhanced documentation for ValueParser classThe documentation for
ValueParser<V>has been improved to better explain its purpose of converting source string values to specific option value types. This makes the API more self-documenting and easier to understand.
92-93: Clarified type parameter documentation in ConfigOptionBaseThe documentation now explicitly explains that
[V]represents the type of value the option provides and how option values are parsed to this type using theValueParser. This improvement helps developers understand the type system used in the configuration framework.lib/src/better_command_runner/config_resolver.dart (3)
14-16: Added helpful context for ignoreUnexpectedPositionalArgs parameterGood documentation addition that explains when to use the new parameter, making it clear that it should be set to true when used from global command runners or commands with subcommands.
22-22: Added optional parameter for ignoring unexpected positional argumentsThis change adds flexibility to the configuration resolution process by allowing commands to ignore unexpected positional arguments when appropriate. This is particularly useful for command runners and parent commands, supporting the hierarchical command structure.
39-39: Implemented ignoreUnexpectedPositionalArgs in DefaultConfigResolverThe implementation properly forwards the new parameter to the
Configuration.resolvemethod, ensuring that the flag is respected throughout the configuration resolution process.Also applies to: 45-45
example/main.dart (1)
26-36:Details
❓ Verification inconclusive
Logger is configured after the command has completed
Because
await commandRunner.run(args)is awaited before the logger is instantiated, any log output that a sub-command tries to emit throughStdOutLoggercreated here will never be seen.
If the intention is to change the global log level before the command executes, construct the logger (or at least call_setLogLevel) prior to invokingrun.- try { - await commandRunner.run(args); - } on UsageException catch (e) { - ... - } - - /// Simple example of using the [StdOutLogger] class. + /// Configure logger *before* running the command so sub-commands inherit it. final LogLevel logLevel; ... + var logger = StdOutLogger(logLevel); + + try { + await commandRunner.run(args); + } on UsageException catch (e) { + ... + }
🏁 Script executed:
#!/bin/bash set -e # Locate the example/main.dart file file=$(fd example/main.dart) echo "Inspecting file: $file" # Print the first 200 lines to see context sed -n '1,200p' "$file" # Show the location of the commandRunner.run call rg -n "await commandRunner.run" -A3 -B3 "$file" # Show where StdOutLogger is instantiated rg -n "StdOutLogger" -A3 -B3 "$file"Length of output: 148
#!/bin/bash set -e # Search for StdOutLogger usages across the repo echo "Occurrences of StdOutLogger:" rg -n "StdOutLogger" -C3 || echo "No matches for StdOutLogger" # Search for commandRunner.run usages echo -e "\nOccurrences of await commandRunner.run:" rg -n "await commandRunner.run" -C3 || echo "No matches for commandRunner.run" # As a fallback, locate any main.dart files echo -e "\nListing all .dart files containing a main function:" rg -l "void main" --glob "*.dart"
#!/bin/bash set -e # List all main.dart files in the repo echo "Listing all main.dart files:" fd main.dart # For each main.dart, show context around logger and run calls for file in $(fd main.dart); do echo -e "\n--- Inspecting $file ---" echo "First 200 lines:" sed -n '1,200p' "$file" echo -e "\nContext around await commandRunner.run:" rg -n "await commandRunner.run" -C3 "$file" || echo " No match for commandRunner.run" echo -e "\nContext around StdOutLogger instantiation:" rg -n "StdOutLogger" -C3 "$file" || echo " No match for StdOutLogger" done
Configure logger before running the command
To ensure sub-commands emit logs at the correct level, instantiate
StdOutLogger(or call_setLogLevel) before invokingawait commandRunner.run(args)so they inherit the configured log level.Locations to update:
- example/main.dart: move logger setup above the
run(args)call.Proposed diff:
- try { - await commandRunner.run(args); - } on UsageException catch (e) { - // … - } - - /// Simple example of using the [StdOutLogger] class. + /// Configure logger *before* running the command so sub-commands inherit it. + final LogLevel logLevel; + if (commandRunner.globalConfiguration.value(StandardGlobalOption.verbose)) { + logLevel = LogLevel.debug; + } else if (commandRunner.globalConfiguration + .value(StandardGlobalOption.quiet)) { + logLevel = LogLevel.error; + } else { + logLevel = LogLevel.info; + } + var logger = StdOutLogger(logLevel); + + try { + await commandRunner.run(args); + } on UsageException catch (e) { + // … + }
Isakdl
left a comment
Isakdl
left a comment
There was a problem hiding this comment.
LGTM, but take a look at the bot comments!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
example/main.dart (1)
110-113: Improved handling of small intervals, but could be more explicitThe code now correctly sets a minimum interval of 1 millisecond, which prevents the potential infinite loop issue that was identified in a past review comment. However, silently adjusting the interval might be confusing to users.
Consider adding a log message when the interval needs to be adjusted:
interval ??= (until.difference(start) ~/ length!); if (interval < const Duration(milliseconds: 1)) { + messageOutput.showWarning( + 'Calculated interval was too small. Using minimum interval of 1ms instead.' + ); interval = const Duration(milliseconds: 1); }
🧹 Nitpick comments (1)
example/main.dart (1)
103-119: Consider adding more efficient series generation for large lengthsThe current implementation generates and prints timestamps one by one in a loop. For large series, this might be inefficient.
Consider adding an optimization for large series:
FutureOr<void>? runWithConfig(Configuration<TimeSeriesOption> commandConfig) { var start = DateTime.now(); var until = commandConfig.value(TimeSeriesOption.until); // exactly one of these options is set var length = commandConfig.optionalValue(TimeSeriesOption.length); var interval = commandConfig.optionalValue(TimeSeriesOption.interval); interval ??= (until.difference(start) ~/ length!); if (interval < const Duration(milliseconds: 1)) { interval = const Duration(milliseconds: 1); } + // Optimization for large series + final totalElements = until.difference(start).inMilliseconds ~/ interval.inMilliseconds; + if (totalElements > 1000) { + messageOutput.showWarning('Generating a large series with $totalElements elements'); + } while (start.isBefore(until)) { print(start); start = start.add(interval); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
example/main.dart(1 hunks)lib/src/better_command_runner/better_command_runner.dart(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/better_command_runner/better_command_runner.dart
🔇 Additional comments (2)
example/main.dart (2)
49-89: Well-structured option definitions with strong typing and constraintsThe use of an enum to define command options with proper typing and constraints is excellent. It showcases the type-safety benefits of the BetterCommand framework and demonstrates best practices for CLI option definitions.
8-42: Excellent example of global option handling and dynamic log level adjustmentThis is a great demonstration of how to use global options to control verbosity and dynamically adjust the log level. The approach is clean and follows best practices by centralizing the log level determination based on global options.
Made the default usage / getting started experience with
BetterCommandandBetterCommandRunnersimpler and more consistent.Command/CommandRunnerBetterCommandRunner,BetterCommand, andConfigurationoptions in the example folderSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests