fix!: Resolved raw generic type warnings #62
Conversation
📝 WalkthroughWalkthroughConsolidates Dart CI jobs and updates workflows/publish tag patterns; tightens generic type bounds to V extends Object across cli_tools and config packages; updates typedefs/class signatures and widespread test code to match; removes some analyzer suppression rules and bumps dev dependency constraints and README links. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Matrix as Job Matrix (Dart SDK × package)
participant Job as Job Steps (defaults.run.working-directory = packages/${matrix.package})
GH->>Matrix: start "Dart format check" (matrix: sdks [3.3,3.9])
Matrix->>Job: dart pub get
Job->>Job: dart format --output=none --set-exit-if-changed .
GH->>Matrix: start "Dart Analyze and Test" (matrix: sdks [3.3,3.9] × packages)
Matrix->>Job: dart pub get
Job->>Job: dart analyze --fatal-infos
Job->>Job: dart test
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
christerswahn
force-pushed
the
ces-dev
branch
8 times, most recently
from
b315bba to
4ca43b0
Compare
![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
🔭 Outside diff range comments (3)
.github/workflows/ci.yml (1)
33-54: Critical: continue-on-error hides analyzer/test failuresThe Analyze and Test job runs with
continue-on-error: true, so matrix failures won’t fail CI. This risks merging breaking changes undetected. Suggest either:
- Make the analyze/test job fail the workflow, or
- Allow failures only on non-primary SDKs.
Example fix (strict, preferred):
dart_analyze_test: name: Dart Analyze and Test - continue-on-error: true + continue-on-error: false strategy: fail-fast: false matrix: dart: [3.3, 3.9] package: [cli_tools, config]If you want to soft-fail only older SDKs:
continue-on-error: ${{ matrix.dart != '3.9' }}packages/cli_tools/test/better_command_runner/default_flags_test.dart (1)
5-12: Add explicit generics to BetterCommandRunner instantiations in testsThe ripgrep output shows two raw (unparameterized) instantiations that need to be updated:
• packages/cli_tools/test/better_command_runner/command_test.dart:42
Change
dart runner = BetterCommandRunner('test', 'this is a test cli')
to
dart runner = BetterCommandRunner<OptionDefinition<Object>, void>( 'test', 'this is a test cli', )• packages/cli_tools/test/better_command_runner/command_test.dart:106
Change
dart runner = BetterCommandRunner( 'test', 'this is a test cli', )
to
dart runner = BetterCommandRunner<OptionDefinition<Object>, void>( 'test', 'this is a test cli', )This will remove all remaining raw generic usages of BetterCommandRunner in your tests.
packages/cli_tools/test/better_command_runner/better_command_test.dart (1)
160-166: Fix raw generic in globalOptions empty listThe empty list literal uses a raw OptionDefinition type, which is inconsistent with the runner’s type parameter and may cause analyzer errors with stricter lints.
Apply this diff:
- globalOptions: <OptionDefinition>[], + globalOptions: <OptionDefinition<Object>>[],
🧹 Nitpick comments (26)
packages/cli_tools/test/pub_api_client_test.dart (3)
16-19: Prefer Future.delayed over wrapping an async body in Future(...)This avoids scheduling an extra microtask and is more idiomatic/concise.
Apply:
- return Future<http.Response>(() async { - await Future<void>.delayed(responseDelay); - return http.Response(body, status); - }); + return Future<http.Response>.delayed( + responseDelay, + () => http.Response(body, status), + );
12-12: Use Duration.zero for clarityMinor readability nit.
Apply:
- final Duration responseDelay = const Duration(seconds: 0), + final Duration responseDelay = Duration.zero,
52-54: Test description contradicts the assertionThe test message says “then returns null” but the assertion expects an exception. Align the description to avoid confusion.
Apply:
- 'Empty body and not found status response when fetching latest version then returns null.', + 'Empty body and not found status response when fetching latest version then throws version fetch exception.',README.md (1)
15-18: Hyphenate “command-line” in proseMinor grammar nit to match common style in technical docs.
Apply:
-configuration ingestion and validation, including typed command line options, +configuration ingestion and validation, including typed command-line options,packages/config/lib/src/config/option_resolution.dart (1)
3-3: Optional: Document the non-null generic constraint for clarityConsider a brief doc comment clarifying that
Vis non-nullable, whilevalueis nullable to represent absence.+/// Resolution of an option where V is a non-null value type. +/// Note: [value] is nullable (V?) to represent absence/error states. final class OptionResolution<V extends Object> {packages/cli_tools/test/test_utils/prompts/option_matcher.dart (2)
39-44: Nit: mark unused matchState parameter to silence lintsSince
matchStateisn’t used, consider prefixing with_to satisfy unused parameter lints in stricter configurations.- Description describeMismatch( + Description describeMismatch( final dynamic item, final Description mismatchDescription, - final Map<dynamic, dynamic> matchState, + final Map<dynamic, dynamic> _matchState, final bool verbose, ) {
84-90: Nit: mark unused matchState parameterSame suggestion to prefix with
_for consistency.- Description describeMismatch( + Description describeMismatch( final dynamic item, final Description mismatchDescription, - final Map<dynamic, dynamic> matchState, + final Map<dynamic, dynamic> _matchState, final bool verbose, ) {packages/config/lib/src/config/options.dart (1)
699-718: Optional: Replace raw generic containers with OptionDefinition / OptionResolutionSeveral APIs still accept raw
OptionDefinition/OptionResolutioncollections. SinceV extends Objectand these types are covariant inV, you can remove raw-type lint warnings by specifyingObject:
- Line 60:
void validateDefinitions(final List<OptionDefinition<Object>> options) {}- Line 67:
String? validateValues(final Map<OptionDefinition<Object>, OptionResolution<Object>> optionResolutions)- Line 715:
void prepareOptionsForParsing(final Iterable<OptionDefinition<Object>> options, ...)- Line 723:
Iterable<OptionDefinition<Object>> validateOptions(final Iterable<OptionDefinition<Object>> options)- Line 725/726/727/729: update maps to
<String, OptionDefinition<Object>>,<int, OptionDefinition<Object>>,<String, OptionDefinition<Object>>, and<OptionGroup, List<OptionDefinition<Object>>>- Line 794:
return argNameOpts.values.cast<OptionDefinition<Object>>();- Line 798:
void addOptionsToParser(final Iterable<OptionDefinition<Object>> argNameOpts, ...)- Line 806:
extension PrepareOptions on Iterable<OptionDefinition<Object>> { ... }This keeps the API heterogenous while avoiding raw generic warnings.
Also applies to: 722-796, 797-805, 806-818
.github/workflows/ci.yml (1)
13-19: CI hygiene: format job can remain non-blocking; consider cachingKeeping the format job non-blocking is fine. For performance, consider enabling Dart pub cache across steps to speed
dart pub get. Not a blocker.Also applies to: 21-23, 30-32
packages/cli_tools/test/test_utils/mock_stdout.dart (1)
28-31: Consider implementing addStream to better mimic Stdout behavior in testsRight now this throws UnimplementedError. If any test starts piping a stream into MockStdout, it’ll fail unexpectedly. A minimal implementation is straightforward and keeps the mock useful.
- Future<dynamic> addStream(final Stream<List<int>> stream) { - throw UnimplementedError(); - } + Future<dynamic> addStream(final Stream<List<int>> stream) async { + // Consume the stream and append decoded content to the buffer. + // We respect the current encoding to mirror IOSink/Stdout semantics. + _buffer.write(await stream.transform(encoding.decoder).join()); + }packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (3)
122-139: Doc sample: update generic bound to V extends ObjectThe example enum still shows V without the Object bound. Align the docs with the new constraint to ensure copy-pasteable code.
-/// enum BespokeGlobalOption<V> implements OptionDefinition<V> { +/// enum BespokeGlobalOption<V extends Object> implements OptionDefinition<V> {
72-74: Typo: “gloabl” → “global”Minor doc typo.
- /// The gloabl option definitions. + /// The global option definitions.
316-325: Optional: give _determineLogLevel a typed Configuration to avoid raw genericThis private helper still uses a raw Configuration, which may reintroduce lint warnings. You can keep it generic but typed to OptionDefinition.
- static CommandRunnerLogLevel _determineLogLevel(final Configuration config) { + static CommandRunnerLogLevel _determineLogLevel( + final Configuration<OptionDefinition<Object>> config, + ) {packages/cli_tools/test/better_command_runner/logging_test.dart (2)
6-28: Preferstatic constfor command name.The command name is immutable; making it const prevents accidental mutation and communicates intent.
Apply:
-class MockCommand extends Command<void> { - static String commandName = 'mock-command'; +class MockCommand extends Command<void> { + static const String commandName = 'mock-command';
47-52: Minor: simplify async setUp lambdas.
setUp(() async => await runner.run([]));can be simplified;setUpaccepts a function returning Future.Apply:
- setUp(() async => await runner.run([])); + setUp(() => runner.run([]));packages/cli_tools/test/better_command_runner/parse_log_level_test.dart (3)
6-17: Preferstatic constfor command name.Use const for immutable metadata.
Apply:
-class MockCommand extends Command<void> { - static String commandName = 'mock-command'; +class MockCommand extends Command<void> { + static const String commandName = 'mock-command';
47-49: UseisNullmatcher for clarity.
expect(parsedCommandName, isNull)communicates intent better than comparing to null.Apply:
- test('then no command passed to setLogLevel.', () { - expect(parsedCommandName, null); - }); + test('then no command passed to setLogLevel.', () { + expect(parsedCommandName, isNull); + });(Repeat the same change for the other occurrences in this file.)
Also applies to: 60-62, 73-75, 89-91
55-56: Minor: simplify async setUp lambdas.Remove redundant
async => await.Example:
- setUp(() async => await runner.run(args)); + setUp(() => runner.run(args));(Apply similarly to other setUps using the same pattern.)
Also applies to: 68-69, 84-85, 124-125, 140-141
packages/cli_tools/test/better_command_runner/analytics_test.dart (5)
10-21: Preferstatic constfor command names.These identifiers are immutable.
Apply:
-class MockCommand extends Command<void> { - static String commandName = 'mock-command'; +class MockCommand extends Command<void> { + static const String commandName = 'mock-command'; @@ -class CompletableMockCommand extends Command<void> { - static String commandName = 'completable-mock-command'; +class CompletableMockCommand extends Command<void> { + static const String commandName = 'completable-mock-command';Also applies to: 31-54
37-43: Make theCompleterfieldfinal.It isn’t reassigned and should be immutable for clarity.
Apply:
- Completer<void> completer = Completer<void>(); + final Completer<void> completer = Completer<void>();
95-109: Stabilize event list handling across tests.Prefer keeping a single list instance and clearing it in tearDown; avoids any subtlety with closures capturing the variable.
Apply:
- List<String> events = []; + final List<String> events = []; @@ - tearDown(() { - events = []; - }); + tearDown(() { + events.clear(); + });
208-223: Same as above: use a single list instance and clear in tearDown.Keeps closures predictable and reduces allocations.
Apply:
- List<String> events = []; + final List<String> events = []; @@ - tearDown(() { - events = []; - }); + tearDown(() { + events.clear(); + });
289-303: Optionally await queue drain after completing the hanging command.You already flush before completion to assert analytics; adding a final flush after
complete()helps avoid inter-test leakage if any microtasks remain (very minor).Apply:
expect(events, hasLength(1)); expect(events.first, equals(CompletableMockCommand.commandName)); command.completer.complete(); + await flushEventQueue();Repeat similarly in the second test after
command.completer.complete();.Also applies to: 325-343
packages/config/lib/src/config/configuration.dart (2)
204-221: Consider guarding against mismatched generic requests in findValueOfCurrent behavior will throw a TypeError if the caller requests a value with the wrong V (e.g., requesting int for a String option). If you prefer returning null (as per the doc: “Returns null if the option is not found or is not set.”), add a runtime guard.
Apply this diff if you want to fail “softly” on type mismatch:
V? findValueOf<V extends Object>({ final String? enumName, final String? argName, final int? argPos, final String? envName, final String? configKey, }) { final option = findOption( enumName: enumName, argName: argName, argPos: argPos, envName: envName, configKey: configKey, ); - if (option == null) return null; - return optionalValue<V>(option as OptionDefinition<V>); + if (option == null) return null; + if (option is! OptionDefinition<V>) return null; + return optionalValue<V>(option); }If you prefer surfacing incorrect usage, keeping the current throwing behavior is fine; consider clarifying that a TypeError may be thrown when V doesn’t match the option’s value type.
273-295: Return type of _getOptionResolution could be generic (optional)You return a raw OptionResolution, which may reintroduce raw generic warnings depending on lint settings. If OptionResolution is generic, you could return OptionResolution and cast the map lookup accordingly. This is purely optional and may not be worth the churn if OptionResolution instances are intentionally untyped at storage.
Possible refactor:
- OptionResolution _getOptionResolution<V extends Object>( - final OptionDefinition<V> option, - ) { + OptionResolution<V> _getOptionResolution<V extends Object>( + final OptionDefinition<V> option, + ) { if (!_options.contains(option)) { throw ArgumentError( '${option.qualifiedString()} is not part of this configuration'); } - final resolution = _config[option]; + final resolution = _config[option] as OptionResolution<V>?; if (resolution == null) { throw OptionDefinitionError(option, 'Out-of-order dependency on not-yet-resolved ${option.qualifiedString()}'); } if (resolution.error != null) { throw InvalidParseStateError( 'No value available for ${option.qualifiedString()} due to previous errors'); } return resolution; }Only adopt if it doesn’t ripple through other APIs or create inference issues when constructing OptionResolution instances.
packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (1)
78-98: Typo in description: “vegatables” → “vegetables”Minor spelling fix in a user-facing string.
Apply this diff:
- final String description = 'Add or remove vegatables to your dish.'; + final String description = 'Add or remove vegetables to your dish.';📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (35)
.github/workflows/ci.yml(1 hunks).github/workflows/publish-cli_tools.yaml(1 hunks).github/workflows/publish-config.yaml(1 hunks)README.md(1 hunks)packages/cli_tools/analysis_options.yaml(0 hunks)packages/cli_tools/example/main.dart(2 hunks)packages/cli_tools/example/simple_command_example.dart(2 hunks)packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart(4 hunks)packages/cli_tools/lib/src/documentation_generator/documentation_generator.dart(1 hunks)packages/cli_tools/pubspec.yaml(1 hunks)packages/cli_tools/test/better_command_runner/analytics_test.dart(7 hunks)packages/cli_tools/test/better_command_runner/better_command_test.dart(12 hunks)packages/cli_tools/test/better_command_runner/command_test.dart(2 hunks)packages/cli_tools/test/better_command_runner/default_flags_test.dart(1 hunks)packages/cli_tools/test/better_command_runner/exit_exceptions_test.dart(2 hunks)packages/cli_tools/test/better_command_runner/logging_test.dart(2 hunks)packages/cli_tools/test/better_command_runner/parse_log_level_test.dart(3 hunks)packages/cli_tools/test/documentation_generator/generate_markdown_test.dart(7 hunks)packages/cli_tools/test/pub_api_client_test.dart(1 hunks)packages/cli_tools/test/test_utils/mock_stdin.dart(1 hunks)packages/cli_tools/test/test_utils/mock_stdout.dart(2 hunks)packages/cli_tools/test/test_utils/prompts/option_matcher.dart(4 hunks)packages/config/README.md(1 hunks)packages/config/analysis_options.yaml(0 hunks)packages/config/example/config_file_example.dart(1 hunks)packages/config/example/main.dart(1 hunks)packages/config/lib/src/config/configuration.dart(5 hunks)packages/config/lib/src/config/option_resolution.dart(1 hunks)packages/config/lib/src/config/option_types.dart(1 hunks)packages/config/lib/src/config/options.dart(5 hunks)packages/config/pubspec.yaml(1 hunks)packages/config/test/config/args_compatibility/command_runner_test.dart(1 hunks)packages/config/test/config/args_compatibility/test_utils.dart(1 hunks)packages/config/test/config/configuration_test.dart(4 hunks)pubspec.yaml(1 hunks)💤 Files with no reviewable changes (2)
- packages/cli_tools/analysis_options.yaml
- packages/config/analysis_options.yaml
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-12T14:55:38.006Z
Learnt from: christerswahn PR: serverpod/cli_tools#47 File: lib/src/config/options.dart:552-567 Timestamp: 2025-06-12T14:55:38.006Z Learning: The codebase relies on a recent version of `package:args` where `ArgParser.addFlag` accepts the `hideNegatedUsage` parameter.Applied to files:
packages/config/test/config/args_compatibility/test_utils.dart📚 Learning: 2025-08-07T07:55:17.269Z
Learnt from: christerswahn PR: serverpod/cli_tools#57 File: packages/config/test/better_command_runner/default_flags_test.dart:1-1 Timestamp: 2025-08-07T07:55:17.269Z Learning: In the `config` package, `better_command_runner.dart` is intentionally kept as a separate import (`package:config/better_command_runner.dart`) rather than being re-exported through the main `packages/config/lib/config.dart` barrel file. This separation is by design according to the package maintainer christerswahn.Applied to files:
packages/cli_tools/test/better_command_runner/command_test.dartpackages/cli_tools/lib/src/documentation_generator/documentation_generator.dartpackages/config/test/config/args_compatibility/command_runner_test.dartpackages/cli_tools/test/better_command_runner/logging_test.dartpackages/cli_tools/test/better_command_runner/default_flags_test.dartpackages/cli_tools/test/better_command_runner/parse_log_level_test.dartpackages/cli_tools/test/better_command_runner/exit_exceptions_test.dartpackages/cli_tools/example/main.dartpackages/cli_tools/test/better_command_runner/analytics_test.dart📚 Learning: 2025-08-06T10:01:41.294Z
Learnt from: SandPod PR: serverpod/cli_tools#54 File: .github/workflows/publish-cli_tools.yaml:6-8 Timestamp: 2025-08-06T10:01:41.294Z Learning: GitHub Actions push.tags filters use glob patterns with a "regular-expression-like subset" that supports quantifiers like `+`, `?`, and character ranges like `[0-9]+`. This means patterns like `v[0-9]+.[0-9]+.[0-9]+` are valid and will match tags correctly, contrary to pure glob pattern rules.Applied to files:
.github/workflows/publish-config.yaml.github/workflows/publish-cli_tools.yaml📚 Learning: 2025-05-02T12:40:13.755Z
Learnt from: christerswahn PR: serverpod/cli_tools#41 File: lib/src/better_command_runner/better_command.dart:55-61 Timestamp: 2025-05-02T12:40:13.755Z Learning: In BetterCommand, null is a valid initializer for messageOutput, and when initialized with null, the getter should return null rather than inherit from the runner's instance. This allows users to explicitly opt out of message output.Applied to files:
packages/cli_tools/test/better_command_runner/parse_log_level_test.dartpackages/cli_tools/test/better_command_runner/analytics_test.dartpackages/cli_tools/test/better_command_runner/better_command_test.dart🪛 LanguageTool
README.md
[grammar] ~15-~15: There might be a mistake here.
Context: ...packages/config/) provides comprehensive configuration ingestion and validation, ...(QB_NEW_EN)
🔇 Additional comments (57)
packages/config/pubspec.yaml (1)
27-29: Dev-dependency bumps look consistent and aligned with the PR goalsserverpod_lints >=2.7.0 and test/test_descriptor updates are consistent with tightening generics and CI upgrades. No concerns from me.
pubspec.yaml (1)
19-20: Confirm melos <7.1.0 compatibility across your workflowsYou widened melos to include 7.0.x. Ensure your melos.yaml and CI tasks are compatible with melos 7.0.x, or consider constraining to <7.0.0 if you explicitly want to avoid the next major.
If you want, I can scan your workflows to spot melos invocations and cross-check against melos 7.0 release notes.
packages/cli_tools/pubspec.yaml (1)
28-30: Dev-dependency updates LGTMThese align with the lints and test updates across the PR. No issues spotted.
README.md (1)
11-11: Link targets updated to package roots — LGTMPointing to the package directories is cleaner and avoids hard-linking to README filenames.
Also applies to: 15-15
.github/workflows/publish-config.yaml (1)
6-7: Clarify tag pattern intent in publish-config workflowThe current pattern
# Matches tags like config-v1.2.3 and config-v1.2.3-pre.1 - 'config-v[0-9]+.[0-9]+.[0-9]+-*'actually matches only stable tags (
config-v1.2.3) and does not match pre-release tags (config-v1.2.3-pre.1). Please confirm which behavior you intend:• If you want both stable and pre-release tags (but still exclude build-metadata
+tags), update to two patterns:- # Matches tags like config-v1.2.3 and config-v1.2.3-pre.1 - - 'config-v[0-9]+.[0-9]+.[0-9]+-*' + # Matches tags like config-v1.2.3 and config-v1.2.3-pre.1 (excludes +metadata) + - 'config-v[0-9]+.[0-9]+.[0-9]+' # stable releases + - 'config-v[0-9]+.[0-9]+.[0-9]+-*' # pre-releases• If you only want stable releases, simplify the comment:
- # Matches tags like config-v1.2.3 and config-v1.2.3-pre.1 + # Matches stable tags like config-v1.2.3 (does not match pre-releases) - 'config-v[0-9]+.[0-9]+.[0-9]+-*'• If you only want pre-releases, adjust both pattern and comment to require a suffix (e.g.
'.*-.*') rather than-*and note that in the comment.Once you confirm the desired scope, please apply the corresponding diff above.
.github/workflows/publish-cli_tools.yaml (1)
6-7: Alignpublish-cli_tools.yamltag filters with intended triggersThe current glob under
on.push.tagsrequires a hyphen before*, so it only matches pre-release tags (e.g.cli_tools-v1.2.3-pre.1) and will not match stable tags likecli_tools-v1.2.3, despite the comment. Please confirm which behavior you want and update the comment and patterns in.github/workflows/publish-cli_tools.yaml(lines 6–7) accordingly:• Trigger on both stable and pre-release tags:
- # Matches tags like cli_tools-v1.2.3 and cli_tools-v1.2.3-pre.1 - - 'cli_tools-v[0-9]+.[0-9]+.[0-9]+-*' + # Matches tags like cli_tools-v1.2.3 and cli_tools-v1.2.3-pre.1 + - 'cli_tools-v[0-9]+.[0-9]+.[0-9]+' + - 'cli_tools-v[0-9]+.[0-9]+.[0-9]+-*'• Trigger only on pre-release tags:
- # Matches tags like cli_tools-v1.2.3 and cli_tools-v1.2.3-pre.1 - - 'cli_tools-v[0-9]+.[0-9]+.[0-9]+-*' + # Matches pre-release tags like cli_tools-v1.2.3-pre.1 + - 'cli_tools-v[0-9]+.[0-9]+.[0-9]+-*'
- File needing update:
• .github/workflows/publish-cli_tools.yaml (lines 6–7)Please confirm the desired trigger set and apply the corresponding change.
packages/config/lib/src/config/option_resolution.dart (1)
3-3: LGTM: Tightening generic bound to non-null Object is correctConstraining
Vtoextends Objectremoves raw/nullable generic pitfalls while keepingvaluenullable viaV?. This aligns with the PR’s objective and is a reasonable small breaking change.packages/cli_tools/test/test_utils/mock_stdin.dart (1)
162-164: LGTM: pipe() return type matches Stream’s signatureUpdating to
Future<dynamic>aligns withStream.pipeand removes raw generic warnings. Body staying unimplemented is fine for this test double.packages/cli_tools/test/test_utils/prompts/option_matcher.dart (2)
15-16: LGTM: Explicitly typing matchState removes raw generic usageUsing
Map<dynamic, dynamic>matches the matcher package expectations and eliminates analyzer warnings. No behavior change.
71-72: LGTM: Same here — explicit Map typing is correctMatches the interface and resolves raw generic warnings.
packages/config/lib/src/config/options.dart (4)
22-33: Docs: Good update to show V extends Object in examplesThe example now reflects the non-null Object bound and aligns with the tightened API. Looks consistent with the rest of the changes.
37-39: API tightening: Bound OptionDefinition’s V to ObjectMoving to
V extends Objectis a sensible, minimally breaking improvement that removes raw generic warnings and enforces non-null value types across the API surface.
165-176: API tightening: Bound ConfigOptionBase’s V to ObjectSame as above—this aligns the base class with the broader repo changes and avoids raw/nullable generic pitfalls.
451-456: Type promotion > cast in _resolveConfigValueUsing
if (value is V) ... value: valueleverages promotion and removes an unnecessary cast. Clearer and still type-safe under Dart 3.3–3.9.packages/config/lib/src/config/option_types.dart (1)
164-166: Strengthen bound: Comparable instead of raw ComparableChanging to
V extends Comparable<Object>removes raw generic usage and preserves correctness for types like int, DateTime, and Duration. This is a small breaking change for any custom types implementing rawComparable, but it’s the right direction.packages/config/README.md (1)
93-126: Docs align with the new generic boundSnippet now shows
LogOption<V extends Object>, matching the public API. Good consistency update.packages/config/test/config/configuration_test.dart (4)
15-19: More precise assertion using OptionDefinitionError.messageSwitching from toString() matching to
having((e) => e.message, ...)is cleaner and less brittle.
35-38: Same improvement applied here: message-based assertionConsistent, targeted assertions reduce test fragility.
57-60: Same improvement applied here: message-based assertionMatches the error surface and avoids relying on toString formatting.
1625-1647: Tighten test enum bound to V extends ObjectTest helper now matches the production bound, ensuring the test harness exercises the intended generic constraints.
packages/cli_tools/test/test_utils/mock_stdout.dart (2)
33-43: LGTM: async API return types align with IOSink/StdoutReturning Future for close, done, and flush matches the platform API shape and resolves the raw Future lint.
70-72: LGTM: writeAll now typed as Iterable<Object?>This tightens the type and eliminates the raw Iterable usage while remaining compatible with typical StringSink semantics.
packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (5)
9-10: LGTM: OnBeforeRunCommand now generic over O and TThe typedef now enforces the same type parameters as BetterCommandRunner, improving type-safety for hooks.
58-59: LGTM: BetterCommandRunner now constrained as <O extends OptionDefinition, T>This removes raw generic usage and communicates the non-null Object value domain across the API surface.
66-66: LGTM: _onBeforeRunCommand typed to OnBeforeRunCommand<O, T>Field type now precisely matches the runner’s generics.
147-155: LGTM: onBeforeRunCommand constructor parameter typedConstructor contracts now match the generic typedef and field, preventing accidental raw-type usage.
364-373: LGTM: StandardGlobalOption now uses V extends ObjectConstraining V aligns with the broader Object-bounded OptionDefinition changes.
packages/cli_tools/example/simple_command_example.dart (2)
17-20: LGTM: Explicit BetterCommandRunner genericsUsing BetterCommandRunner<OptionDefinition, void> matches the new runner constraints and clarifies the return type.
32-47: LGTM: ShowOption constrained to V extends ObjectThis removes raw generic warnings and is consistent with OptionDefinition’s value domain.
packages/cli_tools/lib/src/documentation_generator/documentation_generator.dart (2)
1-2: LGTM: Narrow import to OptionDefinitionImporting OptionDefinition explicitly avoids unnecessary namespace exposure.
6-6: LGTM: commandRunner typed as BetterCommandRunner<OptionDefinition, void>This aligns the generator with the new runner API and removes raw types from the public field.
packages/cli_tools/example/main.dart (2)
5-12: LGTM: Explicit BetterCommandRunner generics and curated global optionsThe explicit OptionDefinition and void types remove raw generic usage. Supplying a reduced globalOptions list is clear and intentional.
50-79: LGTM: TimeSeriesOption constrained to V extends ObjectThis resolves raw generic warnings and matches the OptionDefinition contract.
packages/config/test/config/args_compatibility/command_runner_test.dart (1)
1-1: OK to suppress strict_raw_type in this upstream-compat testGiven this file mirrors package:args behavior and intentionally uses raw
CommandRunnerin places, broadening the ignore list to includestrict_raw_typeis a pragmatic way to keep the test stable without diverging from upstream semantics.packages/config/test/config/args_compatibility/test_utils.dart (1)
5-5: Adding strict_raw_type suppression here makes senseThese helpers intentionally use raw generics to reflect package:args compatibility cases, so extending the file-level ignore to include
strict_raw_typeis appropriate and keeps the tests readable.packages/cli_tools/test/better_command_runner/command_test.dart (1)
5-5: Good: explicit generics for BetterCommandRunner and OptionDefinition import
- Importing
OptionDefinitionfrompackage:config/config.dartis correct.- Switching to
BetterCommandRunner<OptionDefinition<Object>, void>strengthens type safety and aligns with the tightened bounds introduced in this PR.No behavioral changes; tests remain clear and deterministic.
Also applies to: 37-37
packages/cli_tools/test/better_command_runner/exit_exceptions_test.dart (1)
3-3: Strongly-typed Command and BetterCommandRunner are aligned with new bounds
MockCommand extends Command<void>removes a raw generic.BetterCommandRunner<OptionDefinition<Object>, void>matches the updated API constraints.- The added
OptionDefinitionimport is correct.These changes resolve raw generic warnings without altering behavior.
Also applies to: 6-6, 32-35
packages/config/example/main.dart (1)
45-45: Bound generic updated to V extends Object — consistent with API tighteningUpdating
TimeSeriesOptiontoenum TimeSeriesOption<V extends Object> implements OptionDefinition<V>matches the broader constraint that option values are non-nullableObjects. Example remains coherent with the library’s generics model.packages/cli_tools/test/better_command_runner/default_flags_test.dart (1)
2-2: LGTM: Explicit generics and import look correct and align with the new Object-bound API.Using BetterCommandRunner<OptionDefinition, void> removes raw generic warnings and is consistent with the tightened bounds in config/cli_tools. The selective import of OptionDefinition from package:config/config.dart is appropriate.
Also applies to: 7-12
packages/config/example/config_file_example.dart (1)
43-64: LGTM: Tightening the enum bound toV extends Objectis consistent with the new OptionDefinition contract.This change ensures non-null option value types across the example, matching the library changes (File and Duration both satisfy Object).
packages/cli_tools/test/better_command_runner/logging_test.dart (1)
3-4: LGTM: Test now uses explicit generics and the OptionDefinition import; runner wiring remains unchanged.BetterCommandRunner<OptionDefinition, void> matches the updated API and keeps the test intent intact.
Also applies to: 34-41
packages/cli_tools/test/better_command_runner/parse_log_level_test.dart (1)
3-4: LGTM: Strongly-typed runner andCommand<void>usage are consistent with the generics update.The changes align with the Object-bound OptionDefinition and updated CLI runner types. No behavioral changes detected.
Also applies to: 6-17, 28-40, 96-107
packages/cli_tools/test/better_command_runner/analytics_test.dart (1)
5-6: LGTM: Tests updated to explicit generics andCommand<void>; analytics wiring preserved.
- Importing OptionDefinition and parameterizing BetterCommandRunner with OptionDefinition, void removes raw generic warnings.
- Adding messageOutput in constructors is consistent with other tests and keeps output deterministic.
Also applies to: 10-21, 31-54, 57-65, 79-85, 98-104, 211-216, 276-281
packages/config/lib/src/config/configuration.dart (3)
20-31: Docs example correctly tightens generic boundThe example now reflects V extends Object, aligning with the public API and avoiding raw generic usage. Looks good.
241-254: value signature and checks align with the new boundsThe non-nullable accessor uses V extends Object consistently and preserves the existing validation semantics. Good.
260-264: optionalValue cast is acceptable given the call site typeGiven the typed OptionDefinition parameter, the cast back to V? is reasonable. No concerns.
packages/cli_tools/test/better_command_runner/better_command_test.dart (5)
7-17: Enum bound tightened to V extends Object — goodThis removes raw generic usage on OptionDefinition and aligns with the rest of the PR.
19-29: MockCommand generic parameters look correctExtending BetterCommand<OptionDefinition, void> matches the updated runner APIs.
41-46: runWithConfig signature matches the runner’s configuration typeUsing Configuration<OptionDefinition> is consistent with the new bounds.
61-66: Runner is correctly parameterizedExplicit BetterCommandRunner<OptionDefinition, void> removes raw generics from the test.
102-102: Explicit Future.delayed is fineMaking the generic explicit is harmless and clearer in tests that wait for asynchronous side-effects.
Also applies to: 112-112, 122-122, 132-132, 143-143
packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (6)
8-22: Command usage is correctThe explicit void type clarifies the command’s return type.
24-38: RemoveSpiceCommand looks goodConsistent with the other command changes.
40-53: AddVegetableCommand looks goodMatches the refactoring pattern.
55-66: SpiceCommand generic parameters are correctExtending BetterCommand<OptionDefinition, void> aligns with the runner and avoids raw generics.
93-98: VegetableCommand.runWithConfig signature is consistentAlready using Configuration<OptionDefinition> — no action needed.
105-111: Runner is correctly parameterizedBetterCommandRunner<OptionDefinition, void> is consistent with the updated API.
packages/cli_tools/test/documentation_generator/generate_markdown_test.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (1)
83-83: Typo in user-facing description: “vegatables” → “vegetables”This string appears in generated documentation; fix to avoid shipping a typo.
- final String description = 'Add or remove vegatables to your dish.'; + final String description = 'Add or remove vegetables to your dish.';
♻️ Duplicate comments (1)
packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (1)
71-73: Fixed override to Configuration<OptionDefinition>This resolves the earlier mismatch flagged in a previous review; the override now matches the base signature.
🧹 Nitpick comments (6)
packages/cli_tools/pubspec.yaml (1)
28-30: Prefer pinning dev lints with a caret to avoid unexpected future major changes.Using an open lower bound for serverpod_lints can introduce breaking lint rules without changes in this repo, potentially failing CI unexpectedly. Suggest pinning with a caret.
- serverpod_lints: '>=2.7.0' + serverpod_lints: ^2.7.0.github/workflows/ci.yml (3)
29-31: Optional: Add a root-level format check if you keep any Dart files outside packages/.If there are any Dart scripts at the repo root (e.g., tool/ scripts), they won’t be covered by the per-package format steps. Consider a lightweight root job to catch those. If everything lives under packages/, you can skip this.
32-52: Consolidating analyze and test per package is a nice simplification. Consider gating on formatting to save CI minutes.Approve the consolidation and use of
--fatal-infos. Optionally make this job depend on the format job so you don’t run analyze/test when formatting fails.- dart_analyze_test: + dart_analyze_test: + needs: dart_format name: Dart Analyze and Test
33-34: Improve job visibility by including matrix params in the job name.This makes it easier to spot which combo failed at a glance.
- name: Dart Analyze and Test + name: Dart Analyze and Test (${{ matrix.package }}, Dart ${{ matrix.dart }})packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (2)
120-121: Rename misnamed variable for clarityThe variable
vegetableCommandOutputactually holdsspice.mdcontent. Rename to avoid confusion.Use:
final spiceCommandOutput = output['spice.md'];in both tests.
Also applies to: 145-146
115-118: Consider asserting content for vegetable.md as wellYou already assert the file exists, but adding a minimal content assertion will catch regressions in the vegetable command docs too. Example:
test('then vegetable output starts with the main command', () { final vegetableOutput = output['vegetable.md']; expect( vegetableOutput, startsWith( '## Usage\n' '\n' '```console\n' 'Add or remove vegetables to your dish.\n', ), ); });📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/ci.yml(1 hunks)packages/cli_tools/pubspec.yaml(1 hunks)packages/cli_tools/test/documentation_generator/generate_markdown_test.dart(7 hunks)packages/config/pubspec.yaml(1 hunks)pubspec.yaml(1 hunks)🚧 Files skipped from review as they are similar to previous changes (2)
- packages/config/pubspec.yaml
- pubspec.yaml
🔇 Additional comments (10)
packages/cli_tools/pubspec.yaml (2)
28-30: Dev tooling bumps look good and align with stricter analysis.Upgrading serverpod_lints and test dependencies is consistent with the raw generic type cleanup and CI matrix up to Dart 3.9. Nothing concerning here.
28-30:configdependency constraint is correct for 0.7.x
Theconfigpackage is at version 0.7.2 andpackages/cli_tools/pubspec.yamlusesconfig: ^0.7.0, which covers all 0.7.x releases. No update is needed at this time.• packages/config/pubspec.yaml (line 2):
version: 0.7.2
• packages/cli_tools/pubspec.yaml (line 20):config: ^0.7.0We can revisit and bump to
^0.8.0onceconfigreaches 0.8.0..github/workflows/ci.yml (1)
13-23: Per-package format with working-directory and matrix is solid.Running
dart formatinside each package directory matches the monorepo structure and avoids cross-package bleed. Good call on fail-fast: false for broader signal.packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (7)
8-22: Explicit Command generic on AddSpiceCommand: LGTMThis removes the raw type and aligns with analyzer expectations.
24-38: Explicit Command generic on RemoveSpiceCommand: LGTMConsistent with the generics tightening across the repo.
40-53: Explicit Command generic on AddVegetableCommand: LGTMEliminates raw type usage and keeps tests strongly typed.
55-66: BetterCommand<OptionDefinition, void> on SpiceCommand: LGTMType parameters are correct and consistent with the new API.
78-91: BetterCommand<OptionDefinition, void> on VegetableCommand: LGTMStrong typing is consistent with the rest of the changes.
93-95: runWithConfig signature is correctly parameterizedReturns FutureOr? and accepts Configuration<OptionDefinition>; matches BetterCommand<K, R> with K=OptionDefinition, R=void.
105-110: Typed BetterCommandRunner<OptionDefinition, void>: LGTMThis removes raw generic warnings and keeps the runner consistent with command types.
In the latest change of the config package, explicit generic type bounds are required. (Likely to be released in config v0.8.0.) This change in cli_tools is backwards compatible with config v0.7.x.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (1)
71-73: runWithConfig now matches base signature (Configuration<OptionDefinition>) — previous mismatch resolvedThis addresses the earlier override incompatibility that used a raw OptionDefinition. Nice fix.
To ensure there are no remaining raw-generic overrides elsewhere, you can run:
#!/bin/bash set -euo pipefail # Show BetterCommand.runWithConfig declarations and nearby generics rg -n -C3 -g "*.dart" -P '\brunWithConfig\s*\(' echo echo "Searching for potential raw generic usages that should be OptionDefinition<Object>..." # Common patterns to catch missed generics in Configuration/BetterCommand/BetterCommandRunner rg -n -C1 -g "*.dart" -P 'Configuration<OptionDefinition>\s*\w' rg -n -C1 -g "*.dart" -P 'BetterCommand<OptionDefinition>\s*[>,]' rg -n -C1 -g "*.dart" -P 'BetterCommandRunner<OptionDefinition>\s*[>,]'🧹 Nitpick comments (13)
.github/workflows/publish-cli_tools.yaml (2)
7-8: Optional: tighten the dash-suffix pattern to require at least one character after '-'As written, '-*' also matches a dangling trailing hyphen (e.g., cli_tools-v1.2.3-). If you want to avoid that edge case, require at least one alphanumeric after the dash.
Apply this diff:
- - 'cli_tools-v[0-9]+.[0-9]+.[0-9]+-*' + - 'cli_tools-v[0-9]+.[0-9]+.[0-9]+-[A-Za-z0-9][A-Za-z0-9.-]*'
6-8: Clarify intent in the comment: '+' build-metadata tags are intentionally excludedThe new patterns purposefully exclude tags like cli_tools-v1.2.3+1 to avoid releases for melos’ tags. Consider reflecting that in the comment to prevent confusion.
Apply this diff:
- # Matches tags like cli_tools-v1.2.3 and cli_tools-v1.2.3-pre.1 + # Matches tags like cli_tools-v1.2.3 and cli_tools-v1.2.3-pre.1 + # Intentionally excludes tags with '+' build metadata (e.g., cli_tools-v1.2.3+1).packages/cli_tools/test/test_utils/mock_stdout.dart (2)
28-30: Prefer minimal passthrough implementation over throwing in addStream()Throwing here makes the mock brittle. A tiny passthrough keeps behavior realistic and unblocks code that legitimately uses addStream on Stdout/IOSink.
Apply this diff:
- Future<dynamic> addStream(final Stream<List<int>> stream) { - throw UnimplementedError(); - } + Future<dynamic> addStream(final Stream<List<int>> stream) { + return stream.forEach(add); + }
38-38: done should reflect the close lifecycleReturning an already-completed Future defeats the purpose of awaiting done. Tie it to a completer that is completed in close().
Apply this diff:
- Future<dynamic> get done => Future.value(); + Future<dynamic> get done => _doneCompleter.future;Additional context (outside range) if not present yet:
import 'dart:async' show Completer; final _doneCompleter = Completer<dynamic>();README.md (1)
15-18: Minor grammar/style nit: hyphenate “command-line”The phrasing is fine, but “command-line” is typically hyphenated in technical docs. Optional polish:
-The [config package](packages/config/) provides comprehensive -configuration ingestion and validation, including typed command line options, +The [config package](packages/config/) provides comprehensive +configuration ingestion and validation, including typed command-line options,packages/config/test/config/configuration_test.dart (1)
1458-1466: Note: raw OptionDefinition key type in map is likely unavoidableYou’re constructing a heterogeneous map of differently typed options; due to invariance, using OptionDefinition isn’t viable. Keeping OptionDefinition as a raw key type here is reasonable in tests. No action needed.
If analyzer warnings persist here, consider a localized ignore with a comment explaining the invariance constraint.
packages/config/lib/src/config/configuration.dart (2)
204-221: Generic bound added to findValueOf is appropriate; consider clarifying thrown error behaviorThe “V extends Object” bound is correct. However, findValueOf calls optionalValue, which can throw InvalidParseStateError when previous parsing errors exist, whereas the doc says “Returns null if … not set.” Consider updating the doc to mention this, or handle that case here.
If you prefer a doc update (minimal change):
/// Returns `null` if the option is not found or is not set. +/// Note: If the option had prior parsing errors, an [InvalidParseStateError] +/// may be thrown by [optionalValue].
260-265: optionalValue: consider documenting InvalidParseStateErrorSimilar to findValueOf, optionalValue can surface InvalidParseStateError from _getOptionResolution. Optional doc tweak:
-/// Returns `null` if the option is not set. +/// Returns `null` if the option is not set. +/// May throw [InvalidParseStateError] if earlier parse errors prevent access.packages/cli_tools/test/better_command_runner/better_command_test.dart (1)
160-166: Avoid raw generic in empty globalOptions listUse a typed empty list to align with the PR’s objective of removing raw generic warnings.
- globalOptions: <OptionDefinition>[], + globalOptions: <OptionDefinition<Object>>[],packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (4)
78-98: Typo in user-facing description: “vegatables” → “vegetables”This string is rendered into markdown; correct the spelling.
Apply this diff:
- final String description = 'Add or remove vegatables to your dish.'; + final String description = 'Add or remove vegetables to your dish.';
93-95: Optional: prefix unused parameter with underscore to satisfy strict lintsIf unused-parameter lints are enabled, consider underscoring the argument name.
- final Configuration<OptionDefinition<Object>> commandConfig, + final Configuration<OptionDefinition<Object>> _commandConfig,
119-142: Variable name nit: this is spice.md, not vegetableRename for clarity.
- final vegetableCommandOutput = output['spice.md']; + final spiceCommandOutput = output['spice.md']; @@ - vegetableCommandOutput, + spiceCommandOutput,
144-179: Variable name nit (second occurrence): spice.md assigned to a “vegetable” variableMirror the rename here.
- final vegetableCommandOutput = output['spice.md']; + final spiceCommandOutput = output['spice.md']; @@ - vegetableCommandOutput, + spiceCommandOutput,📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (35)
.github/workflows/ci.yml(1 hunks).github/workflows/publish-cli_tools.yaml(1 hunks).github/workflows/publish-config.yaml(1 hunks)README.md(1 hunks)packages/cli_tools/analysis_options.yaml(0 hunks)packages/cli_tools/example/main.dart(2 hunks)packages/cli_tools/example/simple_command_example.dart(2 hunks)packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart(4 hunks)packages/cli_tools/lib/src/documentation_generator/documentation_generator.dart(1 hunks)packages/cli_tools/pubspec.yaml(1 hunks)packages/cli_tools/test/better_command_runner/analytics_test.dart(7 hunks)packages/cli_tools/test/better_command_runner/better_command_test.dart(12 hunks)packages/cli_tools/test/better_command_runner/command_test.dart(2 hunks)packages/cli_tools/test/better_command_runner/default_flags_test.dart(1 hunks)packages/cli_tools/test/better_command_runner/exit_exceptions_test.dart(2 hunks)packages/cli_tools/test/better_command_runner/logging_test.dart(2 hunks)packages/cli_tools/test/better_command_runner/parse_log_level_test.dart(3 hunks)packages/cli_tools/test/documentation_generator/generate_markdown_test.dart(7 hunks)packages/cli_tools/test/pub_api_client_test.dart(1 hunks)packages/cli_tools/test/test_utils/mock_stdin.dart(1 hunks)packages/cli_tools/test/test_utils/mock_stdout.dart(2 hunks)packages/cli_tools/test/test_utils/prompts/option_matcher.dart(4 hunks)packages/config/README.md(1 hunks)packages/config/analysis_options.yaml(0 hunks)packages/config/example/config_file_example.dart(1 hunks)packages/config/example/main.dart(1 hunks)packages/config/lib/src/config/configuration.dart(5 hunks)packages/config/lib/src/config/option_resolution.dart(1 hunks)packages/config/lib/src/config/option_types.dart(1 hunks)packages/config/lib/src/config/options.dart(5 hunks)packages/config/pubspec.yaml(1 hunks)packages/config/test/config/args_compatibility/command_runner_test.dart(1 hunks)packages/config/test/config/args_compatibility/test_utils.dart(1 hunks)packages/config/test/config/configuration_test.dart(4 hunks)pubspec.yaml(1 hunks)💤 Files with no reviewable changes (2)
- packages/cli_tools/analysis_options.yaml
- packages/config/analysis_options.yaml
✅ Files skipped from review due to trivial changes (2)
- packages/config/test/config/args_compatibility/command_runner_test.dart
- packages/config/README.md
🚧 Files skipped from review as they are similar to previous changes (23)
- packages/cli_tools/test/better_command_runner/command_test.dart
- packages/config/test/config/args_compatibility/test_utils.dart
- packages/cli_tools/test/better_command_runner/parse_log_level_test.dart
- packages/config/example/main.dart
- packages/config/pubspec.yaml
- packages/config/lib/src/config/option_resolution.dart
- packages/cli_tools/test/test_utils/prompts/option_matcher.dart
- packages/cli_tools/test/test_utils/mock_stdin.dart
- packages/cli_tools/test/pub_api_client_test.dart
- pubspec.yaml
- packages/config/lib/src/config/option_types.dart
- packages/config/example/config_file_example.dart
- packages/cli_tools/test/better_command_runner/default_flags_test.dart
- packages/cli_tools/test/better_command_runner/logging_test.dart
- packages/cli_tools/pubspec.yaml
- packages/cli_tools/lib/src/documentation_generator/documentation_generator.dart
- packages/cli_tools/example/simple_command_example.dart
- packages/cli_tools/example/main.dart
- .github/workflows/publish-config.yaml
- packages/config/lib/src/config/options.dart
- packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart
- packages/cli_tools/test/better_command_runner/analytics_test.dart
- .github/workflows/ci.yml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-07T07:55:17.269Z
Learnt from: christerswahn PR: serverpod/cli_tools#57 File: packages/config/test/better_command_runner/default_flags_test.dart:1-1 Timestamp: 2025-08-07T07:55:17.269Z Learning: In the `config` package, `better_command_runner.dart` is intentionally kept as a separate import (`package:config/better_command_runner.dart`) rather than being re-exported through the main `packages/config/lib/config.dart` barrel file. This separation is by design according to the package maintainer christerswahn.Applied to files:
packages/cli_tools/test/better_command_runner/exit_exceptions_test.dart📚 Learning: 2025-08-06T10:01:41.294Z
Learnt from: SandPod PR: serverpod/cli_tools#54 File: .github/workflows/publish-cli_tools.yaml:6-8 Timestamp: 2025-08-06T10:01:41.294Z Learning: GitHub Actions push.tags filters use glob patterns with a "regular-expression-like subset" that supports quantifiers like `+`, `?`, and character ranges like `[0-9]+`. This means patterns like `v[0-9]+.[0-9]+.[0-9]+` are valid and will match tags correctly, contrary to pure glob pattern rules.Applied to files:
.github/workflows/publish-cli_tools.yaml📚 Learning: 2025-05-02T12:40:13.755Z
Learnt from: christerswahn PR: serverpod/cli_tools#41 File: lib/src/better_command_runner/better_command.dart:55-61 Timestamp: 2025-05-02T12:40:13.755Z Learning: In BetterCommand, null is a valid initializer for messageOutput, and when initialized with null, the getter should return null rather than inherit from the runner's instance. This allows users to explicitly opt out of message output.Applied to files:
packages/cli_tools/test/better_command_runner/better_command_test.dart🪛 LanguageTool
README.md
[grammar] ~15-~15: There might be a mistake here.
Context: ...packages/config/) provides comprehensive configuration ingestion and validation, ...(QB_NEW_EN)
🔇 Additional comments (23)
.github/workflows/publish-cli_tools.yaml (2)
7-8: Good split: avoids '+' build-metadata tags while still allowing pre-release suffixesSwitching to two explicit patterns correctly prevents triggering on melos-style tags like cli_tools-v1.2.3+1 while still matching plain versions and dash-based pre-releases (e.g., -pre.1). Nice.
7-8: Tag Pattern Consistency VerifiedThe
publish-config.yamlworkflow already uses the same plain-version and dash-suffix regex patterns aspublish-cli_tools.yaml:
- Lines 7–8 in both files match
- '…-v[0-9]+.[0-9]+.[0-9]+'and- '…-v[0-9]+.[0-9]+.[0-9]+-*'No changes are needed.
packages/cli_tools/test/test_utils/mock_stdout.dart (3)
41-43: LGTM on flush()Immediate completion is fine for this mock since there’s no asynchronous IO buffering to drain.
70-72: Great: tightened writeAll typing to Iterable<Object?>This removes raw generic warnings while preserving behavior. Good alignment with the PR’s typing goals.
33-35: No.doneusages orMockStdout.close()calls found – no action neededA repository-wide search returned no references to
.doneor calls toMockStdout.close(). Since nothing awaits thedonefuture, wiring it up isn’t required at this time.README.md (1)
11-11: Link target adjustment looks goodPointing to the package directory instead of README.md improves navigation and avoids 404s if filenames change.
packages/cli_tools/test/better_command_runner/exit_exceptions_test.dart (3)
3-3: Scoped import for OptionDefinition is appropriateImporting OptionDefinition explicitly keeps the test’s namespace clean and aligns with the tightened generics used by BetterCommandRunner.
6-6: Extending Command removes raw generic usageGood fix; avoids raw generic warnings and clarifies the command’s result type.
32-35: Typed BetterCommandRunner instantiation matches updated boundsUsing BetterCommandRunner<OptionDefinition, void> is consistent with the library generics and removes raw generic warnings in tests.
packages/config/test/config/configuration_test.dart (2)
15-19: Asserting on error.message instead of toString() is more preciseSwitching expectations to OptionDefinitionError.message reduces brittleness against framework changes to toString() and improves test clarity.
Also applies to: 34-38, 56-60, 1267-1272
1625-1647: Enum bound tightened to V extends Object is correctUpdating _TestOption to “V extends Object” matches the new OptionDefinition bound and resolves raw generic warnings without changing test behavior.
packages/config/lib/src/config/configuration.dart (3)
20-31: Doc sample updated to V extends Object matches APIThe example enum now reflects the non-nullable bound. Good catch to keep docs aligned with the code.
241-254: value contract remains consistent with new boundsThe method enforces mandatory/default semantics before delegating; no functional change introduced by the bound tightening.
273-276: _getOptionResolution generic bound and signature look soundThe method now ties the option’s value type to the resolution access. Returning a raw OptionResolution is acceptable given heterogeneously typed options and aligns with current usage.
packages/cli_tools/test/better_command_runner/better_command_test.dart (4)
7-17: Enum bound tightened and added Int option: good coverage
- Using “V extends Object” resolves raw generic warnings on the enum.
- Adding an IntOption for age broadens help/usage coverage in tests.
19-46: BetterCommand typing and runWithConfig signature updates are correctExtending BetterCommand<OptionDefinition, void> and typing runWithConfig with Configuration<OptionDefinition> removes raw generics and improves type safety in the test scaffolding.
61-66: Runner generics: correct and consistentInstantiating BetterCommandRunner<OptionDefinition, void> is consistent with the tightened bounds across the codebase.
102-103: Explicit Future.delayed is a nice clarity tweakThis clarifies intent and avoids implicit dynamic Future types in tests.
Also applies to: 112-113, 122-123, 132-133, 143-144
packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (5)
8-22: Explicit generic on Command is correctTyping the Arg Command with void aligns with args’ Command pattern and improves analyzer checks.
24-38: LGTM on RemoveSpiceCommandConsistent with the generics tightening; no issues spotted.
40-53: LGTM on AddVegetableCommandClear, typed command; options are defined as expected.
55-69: BetterCommand<OptionDefinition, void> — correct generic tighteningExtending BetterCommand with OptionDefinition is consistent with the repo-wide change and prevents raw generic warnings.
105-111: Typed BetterCommandRunner<OptionDefinition, void> — looks goodRunner generics align with the commands and documentation generator usage.
fix!: Resolved raw generic type warnings in config and cli_tools packages. Allows removal of the temporary exclusion rules in analysis_options.yaml. Technically a small breaking change.
docs: Modified links from main README to the respective packages
ci: Ensure tested up to Dart 3.9
ci: More specific tag pattern for pre-releases; avoids creating releases for melos' tags with '+' syntax
Summary by CodeRabbit