feat!: By default resolve throws informative UsageException #48
Conversation
📝 WalkthroughWalkthroughThis change replaces usage of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CommandRunner
participant Configuration
User->>CommandRunner: Run command (with args)
CommandRunner->>Configuration: resolveNoExcept(options, argResults, env, configBroker)
Configuration-->>CommandRunner: Returns Configuration (with errors if any)
CommandRunner-->>User: Continues execution (handles errors as needed)
sequenceDiagram
participant User
participant Configuration
User->>Configuration: resolve(options, argResults, env, configBroker)
Configuration->>Configuration: resolveNoExcept(...)
Configuration->>Configuration: throwExceptionOnErrors()
alt Errors exist
Configuration-->>User: Throws UsageException
else No errors
Configuration-->>User: Returns Configuration
end
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
🔭 Outside diff range comments (1)
example/config_file_example.dart (1)
63-68: 🛠️ Refactor suggestionConsider surfacing configuration errors before continuing execution
resolveNoExceptwill happily return aConfigurationcontainingerrors.
BecauserunWithConfigdoes not callcfg.throwExceptionOnErrors()(or otherwise inspectcfg.errors), the command may continue with invalid input and only fail later or – worse – behave incorrectly.Configuration<TimeSeriesOption> resolveConfiguration(ArgResults? argResults) { - return Configuration.resolveNoExcept( + final cfg = Configuration.resolveNoExcept( options: options, argResults: argResults, env: envVariables, configBroker: FileConfigBroker(), ); + cfg.throwExceptionOnErrors(); // will print usage + exit with UsageException + return cfg; }This keeps the non-throwing resolution (so tests can still probe
errors) while ensuring the CLI exits early in production runs.
🧹 Nitpick comments (11)
lib/src/config/multi_config_source.dart (1)
3-4: Imports updated correctly; consider lazy regex compilationImports look right.
Minor nit: the constructorregex()recompiles every pattern on each call site. If large or hot, consider caching compiledRegExpoutside or acceptingRegExpkeys directly to avoid repeated compilation.lib/src/config/config_parser.dart (1)
6-8:option_types.dartappears to be an unused importThe file compiles fine without it – all referenced symbols come from
options.dart. An unused import will trigger anunused_importlinter warning.-import 'option_types.dart';Remove or comment-out unless something later in the PR requires it.
lib/src/config/output_formatting.dart (1)
1-11:formatConfigErroris not Unicode-safe and ignores leading whitespace
error[0]returns the first UTF-16 code unit, not the first user-perceived character.- Leading whitespace (
' invalid path') will be upper-cased as whitespace, leaving the first letter lowercase.- The same single-code-unit issue exists when checking the last “character”.
A safer, still lightweight implementation:
String formatConfigError(final String error) { - 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 error; + + final firstRune = String.fromCharCode(trimmed.runes.first).toUpperCase(); + final rest = trimmed.substring(trimmed.runes.first.length); + final suffix = + _isPunctuation(String.fromCharCode(trimmed.runes.last)) ? '' : '.'; + + return '${' ' * (error.length - trimmed.length)}$firstRune$rest$suffix'; }This keeps semantics while handling multi-byte runes and preserving original indentation.
test/config/file_options_test.dart (1)
17-20: Tests validate the non-throwing path only – add coverage for the throwing variantReplacing
Configuration.resolvewithresolveNoExceptis correct for these cases, but we now lack regression tests thatresolve()still throws aUsageExceptionwith the same message set. Consider duplicating a representative test group and calling the throwing variant to keep both behaviours locked down.test/config/config_source_test.dart (1)
245-248: Minor: test name still mentions “creating the configuration” but now asserts onresolveNoExceptthrowingNothing functionally wrong, but renaming the test to include “(non-throwing API should still fail fast on invalid option configuration)” would make the intent clearer to future readers.
lib/src/better_command_runner/better_command.dart (2)
7-8: Prefer a relative import to avoid leaking thesrcnamespaceUsing a
package:import intosrc/makes the symbol part of the public API surface (users can import it).
Since both libraries live inside the same package, a relative path keeps the helper private:-import 'package:cli_tools/src/config/output_formatting.dart'; +import '../config/output_formatting.dart';
124-129: Duplication of error-handling logic – consider delegating to the throwing factory
Configuration.resolve()now already throws aUsageExceptionthat contains the formatted usage text.
By callingresolveNoExcept()and repeating the “collect → format → throw” pattern here, we keep two independent code paths in sync.- return Configuration.resolveNoExcept( + // Let the config layer deal with the exception formatting. + return Configuration.resolve( options: options, argResults: argResults, env: envVariables, );This removes ~10 lines in
run()and centralises responsibility in the config layer.
If keeping custom formatting is intentional, consider extracting the duplicated block in this file and inbetter_command_runner.dartinto a shared helper to avoid the drift risk.lib/src/better_command_runner/better_command_runner.dart (2)
7-8: Samesrcimport privacy concern as inbetter_command.dartFor the same reason, a relative import is preferable:
-import 'package:cli_tools/src/config/output_formatting.dart'; +import '../config/output_formatting.dart';
312-318: Repeated manual error aggregation – can lean onConfiguration.resolve()As in the command class, the runner still performs manual aggregation after switching to
resolveNoExcept().
Unless the CLI absolutely needs its own bullet-list formatting, lettingConfiguration.resolve()throw directly would simplify the code path and guarantee consistency.See previous suggestion for a minimal change.
test/config/configuration_test.dart (1)
125-145: Avoid brittle exact-match onArgParserusage text.
ArgParserformatting may change between Dart versions, causing spurious test failures.
Prefer a substring/regex check, e.g.expect(options.usage, contains('--project')); expect(options.usage, contains('(defaults to "defaultValueFunction")'));lib/src/config/configuration.dart (1)
287-293: Positional-arg resolution order can mis-consume arguments.
_options.sorted((a,b)=>(a.pos??-1)...)puts non-positional options before positional ones (-1 < 0).
If the list mixes both kinds, positional arguments may be consumed too late, leading to “unexpected positional argument” errors.Sort nulls last to guarantee ascending position order:
- final orderedOpts = _options.sorted( - (a, b) => (a.option.argPos ?? -1).compareTo(b.option.argPos ?? -1)); + final orderedOpts = _options.sorted( + (a, b) => (a.option.argPos ?? 1<<20).compareTo(b.option.argPos ?? 1<<20));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
example/config_file_example.dart(1 hunks)lib/src/better_command_runner/better_command.dart(2 hunks)lib/src/better_command_runner/better_command_runner.dart(2 hunks)lib/src/config/config.dart(1 hunks)lib/src/config/config_parser.dart(1 hunks)lib/src/config/config_source_provider.dart(1 hunks)lib/src/config/configuration.dart(6 hunks)lib/src/config/configuration_broker.dart(1 hunks)lib/src/config/exceptions.dart(1 hunks)lib/src/config/file_system_options.dart(1 hunks)lib/src/config/multi_config_source.dart(1 hunks)lib/src/config/option_groups.dart(1 hunks)lib/src/config/option_types.dart(1 hunks)lib/src/config/options.dart(1 hunks)lib/src/config/output_formatting.dart(1 hunks)test/config/config_source_test.dart(7 hunks)test/config/configuration_test.dart(73 hunks)test/config/configuration_type_test.dart(60 hunks)test/config/file_options_test.dart(18 hunks)
🔇 Additional comments (7)
lib/src/config/configuration_broker.dart (1)
4-12: Public-facing API addition looks cleanThe interface is minimal yet expressive, docs are concise, and the generic bound communicates intent. No issues spotted.
lib/src/config/config_source_provider.dart (1)
4-4: Import aligns dependencies after refactorAdding
options.dartis required forOptionDefinition– good catch.
No other changes needed.lib/src/config/file_system_options.dart (1)
5-5: Correct dependency adjustmentSwitching to
options.dartkeeps the file compiling after the split-out. Looks fine.lib/src/config/option_groups.dart (1)
1-3: Updated imports match the new module layout
InvalidOptionConfigurationErrorandOptionGroupnow live in the new files, so the replacements are correct. 👍lib/src/config/config.dart (1)
6-12: Exports look goodNew modules are re-exported cleanly and no duplicate symbols are exposed.
LGTM.test/config/configuration_type_test.dart (1)
20-25: Tests updated appropriatelySwitching to
resolveNoExcept()keeps the assertions intact and exercises the new non-throwing path – good coverage.lib/src/config/options.dart (1)
520-522: Verify minimum SDK version forbool.parseAPI.
bool.parse(String, {bool caseSensitive = true})was added in Dart 3.
If the package’s lower SDK bound is <3.0 this will break builds.
Confirmpubspec.yamland bumpsdkconstraint if necessary or drop the named parameter.
christerswahn
force-pushed
the
throw-usage
branch
from
30d06b3 to
4e0a301
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/src/config/configuration.dart (3)
69-80: Minor doc-block drift.The macro comment still describes the behaviour but doesn’t explicitly mention that the new factory
resolvenow throws by default.-/// {@template Configuration.resolveNoExcept} +/// {@template Configuration.resolveNoExcept} /// Creates a configuration with option values resolved from the provided context. @@ /// {@endtemplate}Consider adding a short “See also: [Configuration.resolve] for the throwing variant.”
115-140: Factory duplicates parameter list – consider DRYing.The factory currently mirrors the full parameter list of
resolveNoExcept, which is easy to let drift. One neat pattern is to forward theMapof named params:-factory Configuration.resolve({ - required final Iterable<O> options, - final ArgResults? argResults, - ... -}) { - final config = Configuration.resolveNoExcept( - options: options, - argResults: argResults, - ... - ); +factory Configuration.resolve({ + required Iterable<O> options, + ArgResults? argResults, + Iterable<String>? args, + Map<String, String>? env, + ConfigurationBroker? configBroker, + Map<O, Object?>? presetValues, + bool ignoreUnexpectedPositionalArgs = false, +}) { + final config = Configuration.resolveNoExcept( + options: options, + argResults: argResults, + args: args, + env: env, + configBroker: configBroker, + presetValues: presetValues, + ignoreUnexpectedPositionalArgs: ignoreUnexpectedPositionalArgs, + ); config.throwExceptionOnErrors(); return config; }Optional, but reduces maintenance surface.
142-152: Clear error list after throwing to avoid double-throw.If a caller catches
UsageExceptionand inspects the sameConfigurationinstance later, another call tothrowExceptionOnErrors()will re-throw the same exception.-throw UsageException(buffer.toString(), usage); +final message = buffer.toString(); +_errors.clear(); +throw UsageException(message, usage);Not strictly required, but prevents surprises in multi-stage workflows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/src/config/configuration.dart(5 hunks)
🔇 Additional comments (3)
lib/src/config/configuration.dart (3)
9-9: Import looks correct.The additional import is necessary for
formatConfigErrorand doesn’t introduce any side effects.
55-58: Good move toresolveNoExcept.Routing
fromValuesthrough the non-throwing constructor matches the new error-handling model.
268-268: Exception type may not fit the scenario.
OptionDefinitionErrorsounds like a definition-time error, yet this branch is hit when an option’s value is requested before its resolution. That’s arguably a parse-state issue rather than a definition issue. Double-check that the new name still conveys the intended semantics.
When
Configurationis used withoutBetterCommand/Runnerits default behavior should be to throw an informative exception on usage errors, rather then expecting the caller to explicitly check for errors and always provide custom error handling. (This also mirrors the behavior ofargspackageArgParser.) The old behavior is still available but now opt-in.The thrown
UsageExceptioncontains the error messages produced by the options resolution, and the usage help text. This exception behavior is consistent with theBetterCommand/Runner.This is a breaking change for direct uses of
Configuration, but not for uses ofBetterCommand/Runner.Depends on PR #47
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests