refactor: Updated BetterCommand* API on learnings from scloud #42
Conversation
📝 WalkthroughWalkthroughThe changes remove the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BetterCommandRunner
participant BetterCommand
participant Configuration
User->>BetterCommandRunner: run(args)
BetterCommandRunner->>Configuration: resolve(args, envVariables)
Configuration-->>BetterCommandRunner: Configuration instance
BetterCommandRunner->>BetterCommand: run()
BetterCommand->>Configuration: resolve(argResults, envVariables)
Configuration-->>BetterCommand: Configuration instance
BetterCommand->>BetterCommand: runWithConfig(configuration)
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
christerswahn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/src/config/configuration.dart (1)
790-798: Preserve option ordering and avoid shared mutable state in the copy-constructor
configuration.optionsreturns the keys of the original_configmap, which may not preserve the original insertion order stored inconfiguration._options.Map.from(configuration._config)performs a shallow copy; ifOptionResolutionever becomes mutable, the twoConfigurationinstances would silently share the same resolution objects.Consider copying the private fields directly and cloning the map entries to guard against future mutability:
Configuration.from({ required final Configuration<O> configuration, -}) : _options = List.from(configuration.options), - _config = Map.from(configuration._config), +}) : _options = List<O>.from(configuration._options), // keeps order + _config = configuration._config.map( + (k, v) => MapEntry(k, OptionResolution<V>.from(v)), // deep copy + ), _errors = List.from(configuration._errors);(Assumes an appropriate
OptionResolution.fromconstructor; otherwise create one or rely oncopyWith.)lib/src/better_command_runner/better_command_runner.dart (2)
73-76: Make the environment map immutable inside the runner
envVariablescurrently references the supplied map (orPlatform.environment), allowing external code to mutate it after the runner is created. To avoid surprising behaviour, wrap it in anUnmodifiableMapView:-import 'dart:io' show Platform; +import 'dart:io' show Platform; +import 'dart:collection' show UnmodifiableMapView; … - final Map<String, String> envVariables; + final Map<String, String> envVariables; … - envVariables = env ?? Platform.environment, + envVariables = UnmodifiableMapView(env ?? Platform.environment),
381-386: Edge case: multibyte first character handling
formatConfigErrorupper-cases the first code unit rather than the first Unicode code-point, e.g."dž test"becomes"Dž test"(incorrect) and emojis are broken. If this matters, usecharacterspackage orStringExtension.capitalize()frompackage:charactersto be Unicode-safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
example/main.dart(0 hunks)lib/cli_tools.dart(1 hunks)lib/src/better_command_runner/better_command.dart(5 hunks)lib/src/better_command_runner/better_command_runner.dart(8 hunks)lib/src/better_command_runner/config_resolver.dart(0 hunks)lib/src/config/configuration.dart(1 hunks)test/documentation_generator/generate_markdown_test.dart(0 hunks)
💤 Files with no reviewable changes (3)
- test/documentation_generator/generate_markdown_test.dart
- example/main.dart
- lib/src/better_command_runner/config_resolver.dart
🔇 Additional comments (14)
lib/cli_tools.dart (1)
3-4: Exports look goodAdding
config.dartand keeping a singledocs_generator.dartexport removes the previous duplication and surfaces the new public API.lib/src/better_command_runner/better_command.dart (13)
2-2: Import added for environment variables supportThe addition of the Platform import supports the new environment variables handling capability.
8-8: Clean import statementThe simplified import is good practice, removing unnecessary complexity.
10-14: Improved class documentationThe enhanced documentation clearly explains the purpose and capabilities of the BetterCommand class.
21-22: New environment variables fieldGood addition for explicit environment variable handling. This makes configuration resolution more flexible and testable.
32-33: Clear documentation for environment parameterThe documentation clearly explains the optional env parameter and its default behavior.
35-38: Enhanced messageOutput documentationImproved documentation clarity with better formatting and section headers.
63-63: Environment variables parameter and initializationGood implementation of the environment variables parameter with a sensible default to Platform.environment.
Also applies to: 66-66
81-82: Type safety improvement for parent getterUsing dynamic typing here helps avoid type conflicts in the inheritance hierarchy while maintaining the correct runtime type. This is a pragmatic solution.
100-102: Clear documentation of error handlingGood documentation update that clearly explains the error handling behavior in the run method.
119-123: Clear documentation for configuration resolutionThe updated documentation clearly indicates that this method can be overridden, which is important for extensibility.
124-128: Simplified configuration resolutionGood simplification by directly using Configuration.resolve instead of a separate ConfigResolver abstraction. This removes an unnecessary layer while maintaining the same functionality.
131-131: Improved method documentationClear documentation that describes the purpose of the runWithConfig method.
109-114:✅ Verification successful
Centralized error handling
Good refactoring to centralize error handling in the run method. This is a better separation of concerns that allows resolveConfiguration to be overridden more easily without losing error handling.
However, there's an implicit dependency on the formatConfigError function which isn't visible in this file. Make sure this function is accessible.
🏁 Script executed:
#!/bin/bash # Check if formatConfigError is defined and accessible grep -r "formatConfigError" --include="*.dart" .Length of output: 398
🏁 Script executed:
#!/bin/bash # Display the top of better_command.dart to check imports sed -n '1,50p' lib/src/better_command_runner/better_command.dartLength of output: 1953
formatConfigError is properly imported and accessible
The refactoring centralizes error handling nicely. The free function
formatConfigErroris defined inbetter_command_runner.dartand is correctly imported via the relative import inbetter_command.dart. No further action needed.
Upon migrating scloud to the new
BetterCommandandBetterCommandRunnera couple of use patterns needed better support:This PR enables these.
The goal is to release a final
0.5.0.betaafter this and then migrate both theserverpodcommand and thescloudcommands to use ut.Summary by CodeRabbit
New Features
Refactor
Chores