revert: Do not force strict generic types #64
Conversation
📝 WalkthroughWalkthroughThis PR relaxes generics across config and CLI tools, simplifying type bounds and removing explicit type arguments. It updates BetterCommandRunner’s typedef and bounds, adjusts examples and tests to non-generic usages, tweaks several test utility method signatures, and adds analyzer settings to allow raw types and ignore select inference errors. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli_tools/lib/src/documentation_generator/documentation_generator.dart (1)
26-34: Probable missing import for.indexedextension.
entries.indexedtypically comes frompackage:collection/collection.dart. Without it, this may fail to analyze/build.Apply:
-import '../../better_command_runner.dart' show BetterCommandRunner; +import '../../better_command_runner.dart' show BetterCommandRunner; +import 'package:collection/collection.dart';packages/config/test/config/configuration_test.dart (1)
1-1: Replace OptionDefinitionError.message assertions with toString() checks.Two tests still assert on
.message— update them to assert one.toString()(e.g..having((e) => e.toString(), 'message', contains('...'))).
- packages/config/test/config/configuration_test.dart:1270–1272
- packages/config/test/config/configuration_test.dart:1440–1442
🧹 Nitpick comments (9)
packages/cli_tools/analysis_options.yaml (1)
3-9: Temporary lint relaxations: add an inline TODO for later reversalThese settings match the PR intent. Add a TODO so we don’t forget to revert when serverpod_lints drops the stricter rules.
Apply this small annotation:
analyzer: language: strict-raw-types: false + # TODO(serverpod/cli_tools#64): Temporary relaxations to support reverting explicit generics. + # Remove once serverpod_lints aligns with Dart defaults. errors: inference_failure_on_instance_creation: ignore inference_failure_on_function_invocation: ignorepackages/config/analysis_options.yaml (1)
3-10: Analyzer relaxations: annotate as temporary and consider future tighteningSettings align with the revert. Add a TODO so we can restore stricter checks later (or scope ignores to tests when feasible).
Suggested annotation:
analyzer: language: strict-raw-types: false + # TODO(serverpod/config#64): Temporary relaxations while reverting explicit generics. + # Revisit once serverpod_lints matches Dart defaults. errors: inference_failure_on_instance_creation: ignore inference_failure_on_function_invocation: ignore inference_failure_on_untyped_parameter: ignorepackages/config/lib/src/config/options.dart (1)
450-454: Remove redundant cast after type check.
valueis already promoted toVbyif (value is V), soas Vis unnecessary.- return OptionResolution( - value: value as V, - source: ValueSourceType.config, - ); + return OptionResolution( + value: value, + source: ValueSourceType.config, + );packages/config/lib/src/config/option_types.dart (1)
164-164: Loosened bound uses raw Comparable; consider a slightly safer bound.Using raw
Comparabledrops static checks oncompareTo. If it doesn’t fight the revert’s goals, considerComparable<Object?>to keep broad compatibility (e.g., DateTime, int) while retaining a generic argument.-class ComparableValueOption<V extends Comparable> extends ConfigOptionBase<V> { +class ComparableValueOption<V extends Comparable<Object?>> extends ConfigOptionBase<V> {packages/cli_tools/example/simple_command_example.dart (1)
32-47: Optional: align example’s generic bound with library (dropextends Object).Keeps the example consistent with the unbounded generic in OptionDefinition.
-enum ShowOption<V extends Object> implements OptionDefinition<V> { +enum ShowOption<V> implements OptionDefinition<V> {packages/cli_tools/test/better_command_runner/exit_exceptions_test.dart (1)
31-33: Quiet test logs for cleanliness (nit).Capture usage/exception output to avoid noisy stderr in CI.
-final runner = BetterCommandRunner('test', 'this is a test cli') - ..addCommand(MockCommand()); +final runner = BetterCommandRunner( + 'test', + 'this is a test cli', + messageOutput: const MessageOutput(), +) + ..addCommand(MockCommand());packages/config/lib/src/config/configuration.dart (2)
204-220: Type inference on findValueOf can default to dynamicCall sites relying on booleans should prefer explicit type args for clarity and static safety, e.g.
config.findValueOf<bool>(argName: 'verbose').
260-264: Add a debug-time type assertion to surface mismatches earlierThe unchecked cast may hide misconfigured option types at runtime. Add an assert to aid debugging without changing release behavior.
V? optionalValue<V>(final OptionDefinition<V> option) { final resolution = _getOptionResolution(option); - - return resolution.value as V?; + assert( + resolution.value == null || resolution.value is V, + 'Resolved value type does not match the option generic type V', + ); + return resolution.value as V?; }packages/cli_tools/test/better_command_runner/better_command_test.dart (1)
100-100: Avoid fixed 100ms sleeps; yield the event loop insteadYielding once is enough and removes flakiness.
- await Future.delayed(const Duration(milliseconds: 100)); + await Future<void>.delayed(Duration.zero);Apply same replacement in packages/cli_tools/test/better_command_runner/better_command_test.dart at lines 100, 110, 120, 130, 141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
packages/cli_tools/analysis_options.yaml(1 hunks)packages/cli_tools/example/main.dart(1 hunks)packages/cli_tools/example/simple_command_example.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart(3 hunks)packages/cli_tools/lib/src/documentation_generator/documentation_generator.dart(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(11 hunks)packages/cli_tools/test/better_command_runner/command_test.dart(1 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(1 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/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)
🧰 Additional context used
🧠 Learnings (3)
📚 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/config/test/config/args_compatibility/command_runner_test.dartpackages/cli_tools/example/main.dartpackages/cli_tools/lib/src/documentation_generator/documentation_generator.dartpackages/cli_tools/example/simple_command_example.dartpackages/cli_tools/test/better_command_runner/command_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/better_command_test.dart
📚 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/better_command_test.dart
🔇 Additional comments (47)
packages/config/test/config/args_compatibility/test_utils.dart (1)
5-5: Lint suppression cleanup looks goodRemoving the strict_raw_type ignore aligns with the new analyzer settings. No further action.
packages/config/test/config/args_compatibility/command_runner_test.dart (1)
1-1: Lint directive update is consistentDropping strict_raw_type from the ignore list matches package-level analyzer config. Looks good.
packages/config/README.md (1)
93-93: Doc sample updated to unbounded generic — LGTMThe
LogOption<V>bound now matches the library API after the revert. No further changes needed.packages/cli_tools/test/test_utils/mock_stdin.dart (1)
162-162: Signature now matches Stream.pipe — good changeReturning
Future(non-generic) matches the core Stream API and avoids unnecessarydynamic.packages/config/lib/src/config/option_resolution.dart (1)
3-3: Removedextends Objectbound — verify nullable generic scenariosThis broadens
Vto allow nullable types. Ensure existing tests coverOptionResolution<String?>(and similar) to confirmvaluehandling andhasValue/isDefaultlogic behave as expected.packages/cli_tools/test/test_utils/prompts/option_matcher.dart (1)
15-15: Override signatures now matchMatcherexactlyUsing raw
MapformatchStatemirrorspackage:test’sMatcherAPI and avoids generic-mismatch noise. Approved.Also applies to: 42-42, 71-71, 88-88
packages/cli_tools/test/test_utils/mock_stdout.dart (1)
28-35: Signatures aligned with Stdout/IOSink; looks good.Return types and
writeAllparameter now match the platform interfaces.Also applies to: 38-43, 70-72
packages/cli_tools/example/main.dart (1)
5-12: Generic removal on BetterCommandRunner is consistent with the revert.No behavior change; improves ergonomics.
packages/cli_tools/test/pub_api_client_test.dart (1)
17-19: Minor cleanup: preferFuture.delayedoverFuture<void>.delayed.Equivalent behavior; clearer.
packages/config/example/config_file_example.dart (1)
43-64: Enum bound relaxed to unboundedV; matches library changes.Keeps examples in sync with the config API.
packages/cli_tools/test/better_command_runner/command_test.dart (1)
36-43: Runner variable now non-generic; consistent with API change.Tests remain clear and focused.
packages/config/example/main.dart (1)
44-73: Enum bound relaxation aligns with the library update.Example stays consistent with the new typing model.
packages/cli_tools/test/better_command_runner/default_flags_test.dart (1)
6-11: LGTM: raw BetterCommandRunner usage matches the revert.Looks correct; tests remain behaviorally identical.
packages/cli_tools/lib/src/documentation_generator/documentation_generator.dart (1)
4-4: LGTM: field narrowed to non-generic BetterCommandRunner.Consistent with the API revert; no behavior change here.
packages/cli_tools/example/simple_command_example.dart (1)
17-20: LGTM: constructor now relies on type inference.packages/cli_tools/test/better_command_runner/logging_test.dart (2)
5-27: LGTM: non-generic Command in tests.
33-41: LGTM: non-generic BetterCommandRunner with captured output.packages/cli_tools/test/better_command_runner/exit_exceptions_test.dart (1)
5-27: LGTM: non-generic Command in tests.packages/cli_tools/test/better_command_runner/analytics_test.dart (1)
9-28: LGTM: migrated commands and runner to non-generic forms.Consistent across scenarios; analytics assertions unchanged.
Also applies to: 30-53, 56-56, 58-63, 78-83, 97-103, 210-216, 275-281
packages/cli_tools/test/better_command_runner/parse_log_level_test.dart (3)
5-16: LGTM: non-generic Command in tests.
27-38: LGTM: runner with setLogLevel callback (non-generic).
95-105: LGTM: runner with registered command (non-generic).packages/config/test/config/configuration_test.dart (4)
15-20: LGTM: assert on OptionDefinitionError via toString().More resilient to API changes.
35-40: LGTM: same for mandatory-with-default case.
58-63: LGTM: same for fromDefault function case.
1628-1650: LGTM: dropextends Objectbound on test enum generic.Matches the relaxed generic in the library.
packages/config/lib/src/config/configuration.dart (3)
20-31: Docs update to unbounded generics looks goodExample now matches the reverted, inference-friendly API.
241-254: value() null-safety flow is correctNon-null preconditions + promotion make the return safe.
273-292: Private resolver signature consistencyDropping the bound on V here keeps it aligned with the public API changes.
packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (5)
57-58: Relaxed bound to O extends OptionDefinitionMatches the revert intent and improves ergonomics.
65-65: Field type updated to non-generic callback — OKConsistent with typedef change.
146-147: Constructor param type updated — OKPublic surface remains source-compatible for callers passing a function.
291-291: Pre-run hook invocation order is sensibleHook runs after analytics scheduling and before the command; awaited to propagate errors — good.
9-9: Public API change: OnBeforeRunCommand dropped generic — add migration noteGood simplification. Add a short migration/changelog entry: callers using typed callbacks (OnBeforeRunCommand) will no longer compile.
Location: packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart
Re-scan repo for typed usages:
rg -nP --type=dart -C2 '\bOnBeforeRunCommand\s*<'packages/cli_tools/test/better_command_runner/better_command_test.dart (6)
19-30: Drop of generics on MockCommand matches API — OK
41-43: runWithConfig signature aligned with non-generic Configuration — OK
59-65: Non-generic BetterCommandRunner construction — OK
159-165: Explicit empty global options list with relaxed generics — OK
208-213: Runner without analytics correctly omits analytics flag in usage — OK
257-263: Runner with bespoke global options — OKCovers the custom enum path alongside the default.
packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (7)
8-22: Commands drop explicit void type — OKTests rely only on side effects; dynamic return is fine here.
24-38: RemoveSpiceCommand non-generic — OK
40-53: AddVegetableCommand non-generic — OK
55-69: SpiceCommand extends non-generic BetterCommand — OK
76-94: VegetableCommand updates mirror SpiceCommand — OK
101-104: Runner instantiation without generics in setup — OKConfirms docs generator works against the relaxed API.
71-74: runWithConfig widened return type — OKBetterCommand now declares FutureOr?; I checked overrides in examples and tests — they are compatible (examples return void for T=void; tests use FutureOr?). No further changes required.
nielsenko
left a comment
nielsenko
left a comment
There was a problem hiding this comment.
LGTM 🎸
.. left a few comments to consider, but feel free to close them directly.
This reverts two previous commits, for the cli_tools and config packages respectively, that forced explicit generic type specification in all usages of the packages' classes. This made for a worse developer experience without adding any value.
By reverting these commits we will be using Dart's default rules.
(Note: Since serverpod_lints currently enables stricter rules than Dart's default, they are explicitly disable here. When serverpod_lints removes these as is planned, this can be removed here as well.)
Summary by CodeRabbit
Refactor
Documentation
Tests
Chores