fix: Lint and docs updates, introduce melos, and new patch releases #61
Conversation
📝 WalkthroughWalkthroughAdds monorepo tooling (melos) and root pubspec; updates .gitignore and README; bumps cli_tools and config package versions/changelogs; moves some imports/exports to relative/package config; adds BetterCommandRunner.envVariables used for configuration resolution; and applies widespread readonly (final) and small formatting/typing adjustments across library and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as BetterCommandRunner
participant Parser as ArgParser/parse
participant Config as Configuration.resolveNoExcept
participant Env as envVariables / Platform.environment
participant Command as Command execution
Runner->>Parser: parse(args) -> ArgResults
Runner->>Config: resolveNoExcept(argResults, env: Env)
Note right of Config: uses Runner.envVariables if set\notherwise Platform.environment
Config-->>Runner: Configuration
Runner->>Command: run command with Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
.gitignore (1)
1-4: Good additions; consider a couple more common ignoresIgnoring .dart_tool/, .idea/, and *.iml is appropriate. Optionally consider also ignoring:
- .vscode/ (VS Code project settings)
- pubspec.lock at repo root (for packages; apps typically commit it)
If desired, append:
+.vscode/ +pubspec.lockpackages/cli_tools/CHANGELOG.md (1)
1-4: Restore top-level “Changelog” header and fix list indentation (MD007)
- Reintroduce a top-level header for consistency and better readability.
- Fix markdownlint MD007 by removing the leading space before the list dash.
Apply:
-## 0.7.1 - - - **CHORE**(cli_tools): Bumped `config` dependency. +# Changelog + +## 0.7.1 + +- **CHORE**(cli_tools): Bumped `config` dependency.README.md (1)
25-33: Document local invocation of melos and minor copy tweak
- Many contributors won’t have melos globally activated. Consider also showing “dart run melos …”.
- Small wording tweak for clarity.
Suggested changes:
-This repo uses [melos](https://melos.invertase.dev/) to aid in development, -test, and publishing. +This repo uses [melos](https://melos.invertase.dev/) to aid in development, +testing, and publishing. -After cloning the repo, run `melos bootstrap` (or `melos bs`) to initialize it -(this will also run `dart pub get`). +After cloning the repo, run `melos bootstrap` (or `melos bs`) to initialize the workspace +(this also runs `dart pub get`). If you prefer not to globally activate melos, you can run +`dart run melos bootstrap`. -Run `melos test` to run all the tests. +Run `melos test` (or `dart run melos test`) to run all the tests.packages/config/CHANGELOG.md (1)
1-4: Fix markdown list indentation (MD007)Remove the leading space before the list dash to satisfy markdownlint and keep formatting consistent.
- - **DOCS**(config): Corrected GitHub links in README. +- **DOCS**(config): Corrected GitHub links in README.packages/config/README.md (2)
58-60: Link target looks correct; tweak wording for clarityThe link to BetterCommandRunner in cli_tools is correct (and aligns with the design to keep it out of the config barrel). Minor copy tweak improves flow.
-[`BetterCommandRunner`](https://github.com/serverpod/cli_tools/blob/main/packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart) -from the [`cli_tools package`](https://pub.dev/packages/cli_tools). +[`BetterCommandRunner`](https://github.com/serverpod/cli_tools/blob/main/packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart) +from the [cli_tools package](https://pub.dev/packages/cli_tools).
72-76: Maintain consistent phrasing around the BetterCommandRunner linkSame minor copy tweak as above to avoid backticks around the package name.
- (see [`BetterCommandRunner`](https://github.com/serverpod/cli_tools/blob/main/packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart) instead) + (see [BetterCommandRunner](https://github.com/serverpod/cli_tools/blob/main/packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart) instead)
📜 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 settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
packages/cli_tools/pubspec.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.gitignore(1 hunks)README.md(1 hunks)melos.yaml(1 hunks)packages/cli_tools/CHANGELOG.md(1 hunks)packages/cli_tools/pubspec.yaml(2 hunks)packages/config/CHANGELOG.md(1 hunks)packages/config/README.md(3 hunks)packages/config/pubspec.yaml(1 hunks)pubspec.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/README.md
🪛 markdownlint-cli2 (0.17.2)
packages/config/CHANGELOG.md
3-3: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
packages/cli_tools/CHANGELOG.md
3-3: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🪛 LanguageTool
packages/config/README.md
[grammar] ~58-~58: There might be a mistake here.
Context: ...mmand_runner/better_command_runner.dart) from the [cli_tools package](https://p...
(QB_NEW_EN)
README.md
[grammar] ~30-~30: There might be a mistake here.
Context: ...tstrap(ormelos bs) to initialize it (this will also run dart pub get`). Ru...
(QB_NEW_EN)
🔇 Additional comments (8)
packages/cli_tools/pubspec.yaml (1)
2-2: Version bump and dependency update look correct
- cli_tools version 0.7.1 and dependency on config ^0.7.2 align with the CHANGELOG and config’s new version.
- No issues spotted with SDK constraints or other deps.
Proceed once CI confirms resolution across the workspace.
Also applies to: 20-20
melos.yaml (3)
1-5: Workspace selection looks correctIncluding all packages under packages/** is standard for Melos workspaces. Name alignment with the root pubspec is good.
6-9: IntelliJ integration intentionally disabledIf this is to avoid auto-generated run configs, this setting is fine. If contributors use IntelliJ and want package run configs, consider turning this on later.
10-13: Changelog generation disabled for version commandThis matches the PR intent to keep workspace-level changelogs off. No issues spotted.
packages/config/README.md (1)
303-304: Example link is accurateThe example path under cli_tools looks right and matches the monorepo structure.
pubspec.yaml (3)
1-7: Root pubspec metadata is appropriate for a workspace
- publish_to: none prevents accidental publishing.
- Repository metadata is complete and helpful.
8-14: Topics are fine even if the root isn’t publishedKeeping topics here is harmless and keeps metadata in sync if publishing ever becomes relevant.
15-19: SDK constraint and Melos dev dependency look goodDart >=3.3.0 aligns with melos ^6.3.3 requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/cli_tools/CHANGELOG.md (2)
1-1: Confirm intent: top-level "Changelog" header removedMost changelogs keep a top-level "# Changelog" header with version sections beneath. If this removal is intentional for consistency across packages, all good; otherwise consider reintroducing it for clarity.
3-3: Fix list indentation and align style with prior entries
- Leading space before "-" triggers markdownlint MD007 (ul-indent).
- Style is inconsistent with prior entries (bold + scoped type). Previous lines use lowercase conventional types without bold or scope.
Suggested change:
- - **CHORE**(cli_tools): Bumped `config` dependency. +- chore: Bumped `config` dependency.packages/config/CHANGELOG.md (2)
1-1: Confirm intent: top-level "Changelog" header removedSame note as in cli_tools: verify that dropping a top-level "# Changelog" header is intentional across the repo.
3-3: Fix list indentation and match existing conventional commit style
- Remove the leading space before "-" (MD007).
- Use lowercase, unscoped type to match existing entries in this file (e.g., "docs: ...").
Suggested change:
- - **DOCS**(config): Corrected GitHub links in README. +- docs: Corrected GitHub links in README.README.md (1)
27-34: Minor grammar/style tweaks and optional install note for melos
- "development, test, and publishing" → "development, testing, and publishing".
- Optional: add a short note on installing melos to reduce setup friction.
Suggested edits:
-This repo uses [melos](https://melos.invertase.dev/) to aid in development, -test, and publishing. +This repo uses [melos](https://melos.invertase.dev/) to aid in development, +testing, and publishing. @@ -After cloning the repo, run `melos bootstrap` (or `melos bs`) to initialize it +After cloning the repo, run `melos bootstrap` (or `melos bs`) to initialize the workspace (this will also run `dart pub get`). Run `melos test` to run all the tests. + +Note: If you don't have melos installed, follow the +[installation guide](https://melos.invertase.dev/getting-started#installation) +or run `dart pub global activate melos`.
📜 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 ignored due to path filters (2)
packages/cli_tools/pubspec.lockis excluded by!**/*.lockpackages/config/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.gitignore(1 hunks)README.md(1 hunks)melos.yaml(1 hunks)packages/cli_tools/.gitignore(1 hunks)packages/cli_tools/CHANGELOG.md(1 hunks)packages/cli_tools/pubspec.yaml(1 hunks)packages/config/.gitignore(1 hunks)packages/config/CHANGELOG.md(1 hunks)packages/config/pubspec.yaml(1 hunks)pubspec.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/cli_tools/.gitignore
- packages/cli_tools/pubspec.yaml
- pubspec.yaml
- packages/config/pubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- melos.yaml
- .gitignore
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~30-~30: There might be a mistake here.
Context: ...tstrap(ormelos bs) to initialize it (this will also run dart pub get`). Ru...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
packages/cli_tools/CHANGELOG.md
3-3: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
packages/config/CHANGELOG.md
3-3: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🔇 Additional comments (1)
packages/config/.gitignore (1)
4-5: Good call adding package-level ignore for pubspec.lockFor Dart packages, the lockfile shouldn’t be committed and pub uses the package-root .gitignore when publishing. This addition is correct and aligns with the other package in the workspace.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (6)
packages/cli_tools/test/local_storage_manager/local_storage_manager_test.dart (3)
35-44: Fix async assertion:returnsNormallywon’t catch async errors
removeFilereturns a Future. UsingreturnsNormallywith a closure only validates that no synchronous error is thrown when creating the Future. It won’t detect asynchronous failures. Await the call (or usecompletes) to exercise the async path.- expect( - () => LocalStorageManager.removeFile( - fileName: fileName, - localStoragePath: localStoragePath, - ), - returnsNormally, - ); + await LocalStorageManager.removeFile( + fileName: fileName, + localStoragePath: localStoragePath, + );
68-79: Fix async assertion: use Future withthrowsAinstead of a closure
storeJsonFilereturns a Future. A closure won’t surface async exceptions. Pass the Future toexpectsothrowsAcan observe the asynchronous failure.- expect( - () => LocalStorageManager.storeJsonFile( - fileName: fileName, - json: invalidJson, - localStoragePath: localStoragePath, - ), - throwsA(isA<SerializationException>()), - ); + expect( + LocalStorageManager.storeJsonFile( + fileName: fileName, + json: invalidJson, + localStoragePath: localStoragePath, + ), + throwsA(isA<SerializationException>()), + );
120-134: Fix async assertion for malformed JSON caseUse the Future-returning call with
throwsAto capture async deserialization errors.- expect( - () => LocalStorageManager.tryFetchAndDeserializeJsonFile< - Map<String, dynamic>>( - fileName: fileName, - localStoragePath: localStoragePath, - fromJson: (final json) => json, - ), - throwsA(isA<DeserializationException>()), - ); + expect( + LocalStorageManager.tryFetchAndDeserializeJsonFile< + Map<String, dynamic>>( + fileName: fileName, + localStoragePath: localStoragePath, + fromJson: (final json) => json, + ), + throwsA(isA<DeserializationException>()), + );packages/cli_tools/lib/src/local_storage_manager/local_storage_manager.dart (1)
15-23: Fix: throw an exception instance & use USERPROFILE (with HOMEDRIVE/HOMEPATH fallback)Verified: Windows uses USERPROFILE (env names are case‑insensitive on Windows); current code throws a Type object instead of an exception instance.
- File: packages/cli_tools/lib/src/local_storage_manager/local_storage_manager.dart (around lines 15–23)
Apply this diff:
- if (Platform.isWindows) { - return Directory(envVars['UserProfile']!); - } else if (Platform.isLinux || Platform.isMacOS) { - return Directory(envVars['HOME']!); - } - throw (UnsupportedPlatformException); + if (Platform.isWindows) { + final userProfile = envVars['USERPROFILE']; + if (userProfile != null && userProfile.isNotEmpty) { + return Directory(userProfile); + } + final homeDrive = envVars['HOMEDRIVE']; + final homePath = envVars['HOMEPATH']; + if (homeDrive != null && homePath != null) { + return Directory('$homeDrive$homePath'); + } + } else if (Platform.isLinux || Platform.isMacOS) { + final home = envVars['HOME']; + if (home != null && home.isNotEmpty) { + return Directory(home); + } + } + throw UnsupportedPlatformException();packages/cli_tools/lib/src/better_command_runner/better_command.dart (1)
70-78: Use identity check for the sentinel MessageOutputRelying on != can break if MessageOutput defines value equality; a user-provided instance “equal” to the sentinel would be treated as “use runner” instead of honoring the override. Using identical makes the sentinel semantics robust.
Apply this diff:
MessageOutput? get messageOutput { - if (_messageOutput != _defaultMessageOutput) { + if (!identical(_messageOutput, _defaultMessageOutput)) { return _messageOutput; } if (runner case final BetterCommandRunner<O, T> runner) { return runner.messageOutput; } return _messageOutput; }packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (1)
257-261: Bug: Analytics toggle persists across runs by nulling the handler.Setting _onAnalyticsEvent = null when analytics is disabled permanently disables analytics for the lifetime of the runner, even if later runs opt-in again. Gate calls instead of mutating the handler.
Refactor to keep the handler intact and conditionally invoke it:
- if (globalConfiguration.findValueOf( - argName: BetterCommandRunnerFlags.analytics) == - false) { - _onAnalyticsEvent = null; - } + final analyticsEnabled = + globalConfiguration.findValueOf(argName: BetterCommandRunnerFlags.analytics) != false; + final onAnalyticsEvent = analyticsEnabled ? _onAnalyticsEvent : null; @@ - unawaited( + unawaited( Future(() async { final command = topLevelResults.command; if (command != null) { @@ - _onAnalyticsEvent?.call( + onAnalyticsEvent?.call( command.name ?? BetterCommandRunnerAnalyticsEvents.invalid, ); return; } @@ - final noUnexpectedArgs = topLevelResults.rest.isEmpty; - if (noUnexpectedArgs) { - _onAnalyticsEvent?.call(BetterCommandRunnerAnalyticsEvents.help); - } + final noUnexpectedArgs = topLevelResults.rest.isEmpty; + if (noUnexpectedArgs) { + onAnalyticsEvent?.call(BetterCommandRunnerAnalyticsEvents.help); + } }), ); @@ - } on UsageException catch (e) { - messageOutput?.logUsageException(e); - _onAnalyticsEvent?.call(BetterCommandRunnerAnalyticsEvents.invalid); + } on UsageException catch (e) { + messageOutput?.logUsageException(e); + if (analyticsEnabled) { + onAnalyticsEvent?.call(BetterCommandRunnerAnalyticsEvents.invalid); + } rethrow; }Note: If you prefer not to introduce analyticsEnabled/onAnalyticsEvent locals, guard each call with the config check.
Also applies to: 263-289, 291-299
🧹 Nitpick comments (37)
packages/config/test/config/configuration_test.dart (2)
6-6: Nit: fix spelling in test description"abbrevation" → "abbreviation".
- group('Given invalid configuration abbrevation without full name', () { + group('Given invalid configuration abbreviation without full name', () {
797-797: Nit: correct “2st-arg” to “2nd-arg” for consistencyPurely cosmetic but improves clarity across tests.
- final args = ['--second', '2st-arg']; + final args = ['--second', '2nd-arg'];- expect(config.optionalValue(secondOpt), equals('2st-arg')); + expect(config.optionalValue(secondOpt), equals('2nd-arg'));Also applies to: 806-806
packages/config/test/config/args_compatibility/command_test.dart (1)
1-2: File-scoped lint suppression is fine; consider narrowing scope laterSuppressing prefer_final_locals for this legacy-style test is reasonable. If you revisit later, prefer local, targeted ignores (or updating individual vars to final) rather than a file-wide directive.
Would you like a quick pass converting safe locals to final here and replacing the file-level ignore with inline ignores only where mutation is required?
packages/cli_tools/CHANGELOG.md (1)
1-3: Fix Markdown list indentation (MD007)There’s an extra leading space before the list item which trips markdownlint (ul-indent). Remove the leading space.
Apply this diff:
- - **CHORE**(cli_tools): Bumped `config` dependency. +- **CHORE**(cli_tools): Bumped `config` dependency.packages/cli_tools/analysis_options.yaml (1)
6-9: Use explicit severities instead of boolean “false” for analyzer errorsDart analyzer expects severities like ignore/warning/error. Using false may work implicitly but is less clear. Recommend switching to ignore for consistency and clarity.
Apply this diff:
- strict_raw_type: false - inference_failure_on_instance_creation: false - inference_failure_on_function_invocation: false + strict_raw_type: ignore + inference_failure_on_instance_creation: ignore + inference_failure_on_function_invocation: ignoreNote: If you adopt this, consider also updating unnecessary_final above to use ignore for consistency.
packages/config/CHANGELOG.md (1)
1-3: Fix Markdown list indentation (MD007)There’s a leading space before the bullet. Remove it to satisfy markdownlint.
Apply this diff:
- - **DOCS**(config): Corrected GitHub links in README. +- **DOCS**(config): Corrected GitHub links in README.README.md (1)
25-33: Polish wording and add an install hint for MelosMinor grammar/clarity tweaks and a helpful step for first-time contributors.
Apply this diff:
### Development workflow -This repo uses [melos](https://melos.invertase.dev/) to aid in development, -test, and publishing. +This repo uses [melos](https://melos.invertase.dev/) to aid in development, +testing, and publishing. + +If you don't have Melos installed, run: +`dart pub global activate melos` -After cloning the repo, run `melos bootstrap` (or `melos bs`) to initialize it -(this will also run `dart pub get`). +After cloning the repo, run `melos bootstrap` (or `melos bs`) to initialize the workspace +(this also runs `dart pub get`). -Run `melos test` to run all the tests. +Run `melos test` to run all tests.packages/config/analysis_options.yaml (1)
11-13: Enable relative imports; also explicitly disable strict-raw-types lint if desired.
prefer_relative_imports: trueis fine. If you meant to disable the strict raw types lint (rather than an analyzer diagnostic), add it here with the correct dashed name.Apply this diff to add the lint:
linter: rules: prefer_relative_imports: true + strict-raw-types: falseNote: Ensure there’s no conflicting rule in
package:serverpod_lints/cli.yaml(e.g.,avoid_relative_lib_imports). If both are enabled, you’ll get contradictory lints. If you want, I can scan the repo for conflicts.pubspec.yaml (1)
8-14: Nit: topics are unused for a private root package.With
publish_to: none, thetopicssection won’t surface on pub.dev. Safe to keep for documentation, but you can drop it to reduce noise if you prefer.packages/cli_tools/test/local_storage_manager/local_storage_manager_test.dart (1)
11-13: Tear down can be made more robust (delete recursively)If a directory or nested structure ends up in
tempDir,deleteSync()withoutrecursive: truemay fail. Prefer recursive deletion to ensure a clean slate across OSes.- tempDir.listSync().forEach((final file) => file.deleteSync()); + for (final entity in tempDir.listSync()) { + entity.deleteSync(recursive: true); + }packages/cli_tools/lib/src/local_storage_manager/local_storage_manager.dart (2)
58-84: Prefer non-blocking I/O to match async API
storeJsonFileisFuture<void>but usescreateSyncandwriteAsStringSync. Switching to async file ops avoids blocking the event loop and better matches the method’s async contract.- if (!file.existsSync()) { - try { - file.createSync(recursive: true); - } catch (e, stackTrace) { - throw CreateException(file, e, stackTrace); - } - } + if (!file.existsSync()) { + try { + await file.create(recursive: true); + } catch (e, stackTrace) { + throw CreateException(file, e, stackTrace); + } + } @@ - try { - file.writeAsStringSync(jsonString); - } catch (e, stackTrace) { - throw WriteException(file, e, stackTrace); - } + try { + await file.writeAsString(jsonString); + } catch (e, stackTrace) { + throw WriteException(file, e, stackTrace); + }
97-117: Async read and stricter JSON shape check
- Use
await file.readAsString()to stay non-blocking.- Validate that the decoded JSON is a map before invoking
fromJsonto produce clearer errors when the file content is valid JSON but not an object.- String fileContent; - try { - fileContent = file.readAsStringSync(); - } catch (e, stackTrace) { - throw ReadException(file, e, stackTrace); - } + String fileContent; + try { + fileContent = await file.readAsString(); + } catch (e, stackTrace) { + throw ReadException(file, e, stackTrace); + } @@ - try { - return fromJson(jsonDecode(fileContent) as Map<String, dynamic>); - } catch (e, stackTrace) { - throw DeserializationException(file, e, stackTrace); - } + try { + final decoded = jsonDecode(fileContent); + if (decoded is! Map<String, dynamic>) { + throw const FormatException('Expected top-level JSON object'); + } + return fromJson(decoded); + } catch (e, stackTrace) { + throw DeserializationException(file, e, stackTrace); + }packages/cli_tools/test/package_version_test.dart (1)
51-59: Tighten time-delta assertions and fix minor typo
- The variable name timeDifferent reads like a typo; consider renaming to timeDifference for clarity.
- The threshold lessThan(Duration(minutes: 1)) is very generous and weakens the test’s ability to catch regressions. A tighter bound (e.g., 1–2 seconds) still avoids flakiness while providing stronger guarantees.
Apply this diff to adopt clearer naming and a tighter tolerance (repeat similarly for the other occurrences in this file):
- final timeDifferent = storedPackageVersion?.validUntil.difference( + final timeDifference = storedPackageVersion?.validUntil.difference( DateTime.now().add( PackageVersionConstants.localStorageValidityTime, ), ); expect( - timeDifferent, - lessThan(const Duration(minutes: 1)), + timeDifference, + lessThan(const Duration(seconds: 2)), reason: 'Successfully stored version should have a valid until time ' 'close to the current time plus the validity time.', );Also applies to: 78-86, 108-116, 134-142
packages/config/lib/src/config/configuration.dart (1)
148-155: Propagate ArgParser error context into per-option resolution (optional)You already add e.message to the global _errors list. Optionally also include it in each per-option OptionResolution to aid downstream diagnostics (e.g., when inspecting individual option states).
Apply this diff:
} on FormatException catch (e) { _errors.add(e.message); - for (final o in _options) { - _config[o] = const OptionResolution.error('Previous ArgParser error'); - } + for (final o in _options) { + _config[o] = OptionResolution.error('ArgParser error: ${e.message}'); + } return null; }packages/cli_tools/test/documentation_generator/generate_markdown_test.dart (2)
114-116: Rename variable for clarity (spice vs vegetable)The variable is named vegetableCommandOutput but reads the spice.md entry, which is misleading.
Apply:
- final vegetableCommandOutput = output['spice.md']; + final spiceCommandOutput = output['spice.md']; expect( - vegetableCommandOutput, + spiceCommandOutput, startsWith(
139-141: Rename variable for clarity (spice vs vegetable)Same nit: the variable refers to spice.md content.
Apply:
- final vegetableCommandOutput = output['spice.md']; + final spiceCommandOutput = output['spice.md']; expect( - vegetableCommandOutput, + spiceCommandOutput, endsWith(packages/cli_tools/lib/src/analytics/analytics.dart (2)
36-49: Mixpanel track API typically expects base64-encoded payload in the data fieldCurrent code posts body: data=$payload with raw JSON; Mixpanel’s /track endpoint usually requires the JSON to be base64-encoded and assigned to data. Consider encoding before posting. Silent failure here would be hard to detect because errors are intentionally swallowed.
Apply:
void track({required final String event}) { - final payload = jsonEncode({ + final payload = jsonEncode({ 'event': event, 'properties': { 'distinct_id': _uniqueUserId, 'token': _projectToken, 'platform': _getPlatform(), 'dart_version': Platform.version, 'is_ci': ci.isCI, 'version': _version, }, }); - _quietPost(payload); + // Mixpanel expects base64-encoded JSON in the 'data' form field. + final encoded = base64.encode(utf8.encode(payload)); + _quietPost(encoded); } @@ Future<void> _quietPost(final String payload) async { try { await http.post( Uri.parse(_endpoint), - body: 'data=$payload', + body: 'data=$payload', headers: { 'Accept': 'text/plain', 'Content-Type': 'application/x-www-form-urlencoded', }, ).timeout(const Duration(seconds: 2));Also applies to: 63-72
52-55: Nit: Platform casing for macOSApple’s platform branding is macOS (lowercase m). Totally minor, but easy to tidy up.
- if (Platform.isMacOS) { - return 'MacOS'; + if (Platform.isMacOS) { + return 'macOS';packages/cli_tools/lib/src/package_version/package_version.dart (2)
22-27: Avoid shadowing the static method name with the callback parameter.The named parameter
fetchLatestPackageVersionshadows the enclosing static method name, which hurts readability and can confuse IDE navigation. Renaming the callback parameter clarifies intent.Apply:
static Future<Version?> fetchLatestPackageVersion({ - required final Future<void> Function(PackageVersionData versionArtefact) - storePackageVersionData, - required final Future<PackageVersionData?> Function() - loadPackageVersionData, - required final Future<Version?> Function() fetchLatestPackageVersion, + required final Future<void> Function(PackageVersionData versionArtefact) + storePackageVersionData, + required final Future<PackageVersionData?> Function() + loadPackageVersionData, + required final Future<Version?> Function() getLatestPackageVersion, }) async { final storedVersionData = await loadPackageVersionData(); @@ - final latestPackageVersion = await fetchLatestPackageVersion(); + final latestPackageVersion = await getLatestPackageVersion();Also applies to: 34-35
73-76: Consider making PackageVersionData immutable.
PackageVersionDataappears as a value object. Making fieldsfinal(and the constructorconst) would better reflect intent and prevent accidental mutation.-class PackageVersionData { - Version version; - DateTime validUntil; - - PackageVersionData(this.version, this.validUntil); +class PackageVersionData { + final Version version; + final DateTime validUntil; + + const PackageVersionData(this.version, this.validUntil); }Note: Calls using runtime values (e.g.,
DateTime.now()) will still work; they just won’t be const-invoked.packages/cli_tools/test/test_utils/mock_stdin.dart (1)
49-52: Provide a trivial pass-through for cast().Implementing
castusingStream.castFromavoids unexpectedUnimplementedErrorif tests use it and aligns with the standardStreambehavior.- Stream<R> cast<R>() { - throw UnimplementedError(); - } + Stream<R> cast<R>() => Stream.castFrom<List<int>, R>(this);packages/cli_tools/test/prompts/input_test.dart (1)
66-66: Fix typo in test description (“defult” → “default”)Minor nit that makes test names cleaner.
Apply this diff:
- 'when providing defult value ' + 'when providing default value 'packages/cli_tools/test/better_command_runner/logging_test.dart (2)
20-20: Prefer explicit type on callback parameterArgParser.addOption’s callback receives a String?; explicitly annotate to avoid implicit dynamic and improve readability.
Apply this diff:
- callback: (final name) {}, + callback: (final String? name) {},
37-39: Prefer explicit types on logging callbacksGive the closure parameters explicit types to avoid implicit dynamic and document the expected payload.
Apply this diff:
- usageExceptionLogger: (final e) => errors.add(e.toString()), - usageLogger: (final u) => infos.add(u), + usageExceptionLogger: (final Object e) => errors.add(e.toString()), + usageLogger: (final String u) => infos.add(u),packages/cli_tools/test/prompts/select_test.dart (1)
132-132: Optional: annotate type of exception in matcher closureExplicitly typing the parameter helps readability and static analysis, since having enforces ExitException.
Apply this diff:
- (final e) => e.exitCode, + (final ExitException e) => e.exitCode,packages/cli_tools/test/better_command_runner/command_test.dart (2)
88-96: Strengthen negative-path assertion (optional)Consider also asserting that no options were tracked to make the failure mode more explicit when parsing fails.
Apply this diff to add one more assertion:
try { await runner.run(args); } catch (_) { // ignore any exceptions. } expect(mockCommand.numberOfRuns, equals(0)); + expect(mockCommand.trackedOptions, isEmpty);
103-110: Prefer microtask for before-run callback scheduling (optional)Using Future.microtask avoids scheduling on the event queue and is slightly cheaper and earlier in ordering than Future(() { ... }). Both work since the runner awaits the Future, but microtask is commonly used for this pattern.
Apply this diff:
- onBeforeRunCommand: (final _) => Future(() => calls.add('callback')), + onBeforeRunCommand: + (final _) => Future.microtask(() => calls.add('callback')),packages/cli_tools/test/better_command_runner/analytics_test.dart (1)
58-64: Local runner shadows outer runner (minor clarity nit)Declaring a local final runner here is fine, but it shadows the outer late BetterCommandRunner. Consider renaming to localRunner or removing the outer variable for this group to reduce shadowing.
packages/cli_tools/test/test_utils/prompts/option_matcher.dart (1)
5-7: Top-level matcher APIs with final parametersGood immutability improvement for equalsOption and containsAllOptions functions.
The name containsAllOptions implies order-insensitive matching, but the implementation enforces order and length equality. Consider renaming to equalsAllOptionsInOrder or adjusting docs to reflect order-sensitivity.
packages/cli_tools/test/prompts/multiple_select_test.dart (2)
21-25: Immutability of test option lists is a nice touchFinalizing these shared lists improves test clarity and prevents accidental reassignment. If Option has a const constructor, consider const literals for additional wins.
Also applies to: 43-47, 104-108, 137-141, 159-163, 187-191, 258-262, 293-296
78-81: Fix test description: refers to “select” but uses multiselectThe description says “select prompt” though the test exercises multiselect. Suggest adjusting wording for consistency.
Apply this diff to fix the description:
- test( - 'Given select prompt ' + test( + 'Given multiselect prompt ' 'when providing options ' 'then instruction should be given last', () async {packages/cli_tools/lib/src/prompts/input.dart (1)
20-27: Avoid shadowing the function name with a local ‘input’ variableThe local variable named input shadows the function identifier, which can be confusing. A small rename improves readability.
Apply this diff:
- final input = stdin.readLineSync()?.trim(); - final missingInput = input == null || input.isEmpty; + final line = stdin.readLineSync()?.trim(); + final missingInput = line == null || line.isEmpty; if (missingInput) { return defaultValue ?? ''; } - return input; + return line;packages/cli_tools/lib/src/logger/loggers/void_logger.dart (1)
50-52: Prefer an immediately completed futureUse Future.value() to return a completed void future explicitly and avoid creating a new microtask unnecessarily.
Apply this diff:
@override Future<void> flush() { - return Future(() => {}); + return Future.value(); }packages/cli_tools/lib/src/logger/helpers/progress.dart (1)
154-161: Spinner first-frame off-by-one (terminal vs non-terminal)In the non-terminal path you render frames.first, but in _onTick you pre-increment _index and start from frames[1]. Aligning both to start at frames[0] yields consistent UX.
Apply this diff:
- void _onTick(final Timer? _) { - _index++; - final frames = _options.animation.frames; - final char = frames.isEmpty ? '' : frames[_index % frames.length]; + void _onTick(final Timer? _) { + final frames = _options.animation.frames; + final char = frames.isEmpty ? '' : frames[_index % frames.length]; + _index = (_index + 1) % (frames.isEmpty ? 1 : frames.length); final prefix = char.isEmpty ? char : '${AnsiStyle.lightGreen.wrap(char)} '; _write('$_clearLine$prefix$_message... $_time'); }packages/cli_tools/lib/src/prompts/select.dart (2)
167-176: Avoid mutating shared Set across state instances.toggleCurrent mutates the same Set instance (selectedOptions..add/remove), which couples old/new state. Prefer immutable-style updates to prevent aliasing bugs.
Apply:
_SelectState toggleCurrent() { - return _SelectState( - options: options, - selectedIndex: selectedIndex, - selectedOptions: selectedOptions.contains(selectedIndex) - ? (selectedOptions..remove(selectedIndex)) - : (selectedOptions..add(selectedIndex)), - multiple: multiple, - ); + final next = Set<int>.of(selectedOptions); + if (next.contains(selectedIndex)) { + next.remove(selectedIndex); + } else { + next.add(selectedIndex); + } + return _SelectState( + options: options, + selectedIndex: selectedIndex, + selectedOptions: next, + multiple: multiple, + ); }
127-133: Improve UX hint to include navigation and quit keys.Current hint doesn’t mention arrow keys or [q]. Consider:
logger.write( - state.multiple - ? 'Press [Space] to toggle selection, [Enter] to confirm.' - : 'Press [Enter] to confirm.', + state.multiple + ? 'Use ↑/↓ to navigate, [Space] to toggle selection, [Enter] to confirm, [q] to quit.' + : 'Use ↑/↓ to navigate, [Enter] to confirm, [q] to quit.', LogLevel.info, newParagraph: true, );packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (1)
71-73: Typo in comment.“The gloabl option definitions.” -> “The global option definitions.”
- /// The gloabl option definitions. + /// The global option definitions.
packages/cli_tools/test/local_storage_manager/local_storage_manager_test.dart
Show resolved
Hide resolved
christerswahn
changed the title
Because we now gitignore the pubspec.lock file it is regenerated from scratch, causing several major version bumps of the Dart analyzer and linter rules. This commit fixes the linter rules and adds explicit ignores to some of the new analyzer rules, which should be addressed separately since they may require changing some approaches to generic type resolution.
- cli_tools@0.7.1 - config@0.7.2
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/cli_tools/test/local_storage_manager/local_storage_manager_test.dart (2)
37-43: Fix async assertion: returnsNormally doesn’t validate Future completion
returnsNormallyonly checks for synchronous exceptions. SinceremoveFilereturns a Future, usecompletes(or simply await the call) so async errors are actually caught.- expect( - () => LocalStorageManager.removeFile( + await expectLater( + LocalStorageManager.removeFile( fileName: fileName, localStoragePath: localStoragePath, ), - returnsNormally, + completes, );
71-78: Fix async throws assertion for storeJsonFileThe function returns a Future; wrapping it in a closure makes
throwsAonly check sync errors. Pass the Future directly toexpectLater.- expect( - () => LocalStorageManager.storeJsonFile( + await expectLater( + LocalStorageManager.storeJsonFile( fileName: fileName, json: invalidJson, localStoragePath: localStoragePath, ), throwsA(isA<SerializationException>()), );
♻️ Duplicate comments (3)
packages/cli_tools/test/local_storage_manager/local_storage_manager_test.dart (2)
144-152: Fix async assertion for corrupt file case (pass Future, not closure)This is the same issue previously flagged—remove the wrapper and pass the Future so
throwsAcan observe the asyncReadException.- await expectLater( - () => LocalStorageManager.tryFetchAndDeserializeJsonFile< + await expectLater( + LocalStorageManager.tryFetchAndDeserializeJsonFile< Map<String, dynamic>>( fileName: fileName, localStoragePath: localStoragePath, fromJson: (final json) => json, ), throwsA(isA<ReadException>()), );
125-133: Fix async throws assertion for malformed JSON caseSame pattern—pass the Future so
throwsAobserves the asynchronous error.- expect( - () => LocalStorageManager.tryFetchAndDeserializeJsonFile< + await expectLater( + LocalStorageManager.tryFetchAndDeserializeJsonFile< Map<String, dynamic>>( fileName: fileName, localStoragePath: localStoragePath, fromJson: (final json) => json, ), throwsA(isA<DeserializationException>()), );packages/cli_tools/lib/src/logger/logger.dart (1)
105-107: Document that BoxLogType.newParagraph is intentionally ignoredPer earlier discussion, you’re keeping newParagraph for backward compatibility. Adding a doc note will prevent confusion for consumers.
Apply this diff:
class BoxLogType extends LogType { final String? title; - const BoxLogType({this.title, final bool newParagraph = true}); + /// Kept for backward compatibility. The `newParagraph` parameter is accepted + /// but ignored by current implementations. + const BoxLogType({this.title, final bool newParagraph = true}); }
🧹 Nitpick comments (15)
packages/cli_tools/CHANGELOG.md (2)
3-3: Fix markdown list indentation (MD007).Top-level bullets shouldn’t be indented. Remove the leading space before the dash to satisfy markdownlint.
- - **CHORE**(cli_tools): Bumped `config` dependency. +- **CHORE**(cli_tools): Bumped `config` dependency.
1-1: Consider reinstating a top-level “Changelog” header for consistency.Many projects keep “# Changelog” as the document title with versions as “## …”. Optional, but it improves readability and aligns with common conventions.
-## 0.7.1 +# Changelog + +## 0.7.1packages/config/CHANGELOG.md (2)
3-3: Fix markdown list indentation (MD007).Remove the leading space before the dash for the 0.7.2 entry.
- - **DOCS**(config): Corrected GitHub links in README. +- **DOCS**(config): Corrected GitHub links in README.
1-1: Optional: Add a top-level “Changelog” header.Keeps the changelog structure consistent across files and with common conventions.
-## 0.7.2 +# Changelog + +## 0.7.2README.md (3)
27-29: Tweak phrasing: “test” → “testing”.Minor grammar/style improvement.
-This repo uses [melos](https://melos.invertase.dev/) to aid in development, -test, and publishing. +This repo uses [melos](https://melos.invertase.dev/) to aid in development, +testing, and publishing.
30-33: Clarify wording and tighten grammar.
- “initialize it” → “initialize the workspace”
- “this will also run” → “this also runs”
- “run all the tests” → “execute all tests”
-After cloning the repo, run `melos bootstrap` (or `melos bs`) to initialize it -(this will also run `dart pub get`). +After cloning the repo, run `melos bootstrap` (or `melos bs`) to initialize the workspace +(this also runs `dart pub get`). -Run `melos test` to run all the tests. +Run `melos test` to execute all tests.
25-33: Add quick install instructions for Melos (developer onboarding).New contributors may not have Melos installed. Adding a one-liner improves onboarding DX.
You can insert after introducing Melos:
# Install Melos if you don't have it dart pub global activate melos # Ensure pub global executables are on your PATH (adjust path for your OS/shell) export PATH="$PATH":"$HOME/.pub-cache/bin"If you want, I can propose the exact insertion location as a patch.
packages/cli_tools/test/std_out_logger_test.dart (2)
16-16: Prefer discarding the unused stdin binding over file-wide ignore.You can avoid the file-scoped unused_local_variable ignore by discarding stdin in the record pattern. This narrows the ignore surface and keeps the test tidy.
Apply this diff to each collectOutput destructuring:
- final (:stdout, :stderr, :stdin) = await collectOutput( + final (:stdout, :stderr, stdin: _) = await collectOutput(Also applies to: 26-26, 36-36, 46-46, 56-56, 66-66, 76-76, 86-86, 103-103, 113-113, 123-123, 133-133
2-2: Remove file-wide unused_local_variable ignore after discarding stdin.Once stdin is discarded via the record pattern, the file-level ignore becomes unnecessary and could mask future unused variable mistakes.
Apply:
-// ignore_for_file: unused_local_variablepackages/cli_tools/test/local_storage_manager/local_storage_manager_test.dart (1)
8-13: Ensure robust cleanup and no temp dir leakage
- Current teardown deletes only direct children and doesn’t remove the temp directory after all tests. If a subdirectory is ever created, deleteSync without recursive=true can fail.
- Suggest: recursively delete entities in tearDown and remove the temp dir once in tearDownAll.
Apply this diff to strengthen per-test cleanup:
- tearDown(() { - tempDir.listSync().forEach((final file) => file.deleteSync()); - }); + tearDown(() { + for (final entity in tempDir.listSync()) { + entity.deleteSync(recursive: true); + } + });Then add this once-after-all cleanup (outside the selected lines):
tearDownAll(() { if (tempDir.existsSync()) { tempDir.deleteSync(recursive: true); } });packages/config/lib/src/config/options.dart (1)
59-61: Widen validateDefinitions parameter to Iterable — in-repo overrides OK, but this is a public API change (verify with consumers)Brief: Repo-wide search shows the only in-repo override already accepts Iterable, so widening the base signature is safe inside this repo. However this is a public API change and may break external subclasses/consumers, so do it in a separate PR and coordinate/version accordingly.
Locations found:
- packages/config/lib/src/config/options.dart:60 — current base definition:
void validateDefinitions(final List options) {}- packages/config/lib/src/config/option_groups.dart:35 — override in MutuallyExclusive already uses:
void validateDefinitions(final Iterable options) { ... }- packages/config/lib/src/config/options.dart:728 — optionGroups typed as <OptionGroup, List>
- packages/config/lib/src/config/options.dart:771 — call site: group.validateDefinitions(options);
Proposed diff:
- void validateDefinitions(final List<OptionDefinition> options) {} + void validateDefinitions(final Iterable<OptionDefinition> options) {}Action: Safe to apply within this repo (no in-repo overrides require changes), but verify external consumers/subclasses before landing — perform the change in a separate PR and bump/communicate the API change.
packages/cli_tools/lib/src/package_version/package_version.dart (3)
78-82: fromJson: prefer tolerant cast for epoch millisCasting valid_until as int will throw if a num sneaks in (e.g., JSON decoder yields double). Casting to num and toInt() is more robust without changing behavior.
Apply this diff:
- factory PackageVersionData.fromJson(final Map<String, dynamic> json) => - PackageVersionData( - Version.parse(json['version'] as String), - DateTime.fromMillisecondsSinceEpoch(json['valid_until'] as int), - ); + factory PackageVersionData.fromJson(final Map<String, dynamic> json) => + PackageVersionData( + Version.parse(json['version'] as String), + DateTime.fromMillisecondsSinceEpoch( + (json['valid_until'] as num).toInt(), + ), + );
34-41: Optionally guard against exceptions from the fetcher to honor “unsuccessful fetch” semanticsDocs say an unsuccessful fetch enforces a retry timeout. If the provided fetchLatestPackageVersion throws, we currently propagate the error instead of storing a negative cache. Wrapping in try/catch aligns behavior with the docs without burdening the callback to always swallow errors.
Apply this diff:
- final latestPackageVersion = await fetchLatestPackageVersion(); - - await _storePubDevVersion( - latestPackageVersion, - storePackageVersionData: storePackageVersionData, - ); + Version? latestPackageVersion; + try { + latestPackageVersion = await fetchLatestPackageVersion(); + } catch (_) { + latestPackageVersion = null; + } + + await _storePubDevVersion( + latestPackageVersion, + storePackageVersionData: storePackageVersionData, + );
73-77: Consider making PackageVersionData fields immutableIf external code doesn’t mutate version/validUntil after construction, making them final prevents accidental changes and matches the immutability theme of this PR.
Apply this diff:
-class PackageVersionData { - Version version; - DateTime validUntil; +class PackageVersionData { + final Version version; + final DateTime validUntil; PackageVersionData(this.version, this.validUntil);Note: This is source-compatible unless callers mutate the fields. If they do, treat this as a follow-up change.
packages/cli_tools/test/test_utils/mock_stdin.dart (1)
49-52: Provide a minimal, usable cast() implementationA simple delegation keeps behavior consistent with Stream.cast and avoids test surprises if a typed view is requested.
Apply this diff:
@override Stream<R> cast<R>() { - throw UnimplementedError(); + // Behave like Stream.cast<R>() for compatibility with typed consumers. + return Stream.castFrom<List<int>, R>(this); }
📜 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 (71)
.gitignore(1 hunks)README.md(1 hunks)melos.yaml(1 hunks)packages/cli_tools/CHANGELOG.md(1 hunks)packages/cli_tools/analysis_options.yaml(1 hunks)packages/cli_tools/example/main.dart(3 hunks)packages/cli_tools/example/simple_command_example.dart(2 hunks)packages/cli_tools/lib/logger.dart(1 hunks)packages/cli_tools/lib/package_version.dart(1 hunks)packages/cli_tools/lib/src/analytics/analytics.dart(4 hunks)packages/cli_tools/lib/src/better_command_runner/better_command.dart(3 hunks)packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart(11 hunks)packages/cli_tools/lib/src/documentation_generator/documentation_generator.dart(2 hunks)packages/cli_tools/lib/src/local_storage_manager/local_storage_manager.dart(5 hunks)packages/cli_tools/lib/src/logger/helpers/ansi_style.dart(1 hunks)packages/cli_tools/lib/src/logger/helpers/progress.dart(7 hunks)packages/cli_tools/lib/src/logger/logger.dart(4 hunks)packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart(11 hunks)packages/cli_tools/lib/src/logger/loggers/void_logger.dart(3 hunks)packages/cli_tools/lib/src/package_version/package_version.dart(3 hunks)packages/cli_tools/lib/src/package_version/pub_api_client.dart(4 hunks)packages/cli_tools/lib/src/prompts/confirm.dart(2 hunks)packages/cli_tools/lib/src/prompts/input.dart(1 hunks)packages/cli_tools/lib/src/prompts/select.dart(8 hunks)packages/cli_tools/pubspec.yaml(2 hunks)packages/cli_tools/test/better_command_runner/analytics_test.dart(14 hunks)packages/cli_tools/test/better_command_runner/better_command_test.dart(6 hunks)packages/cli_tools/test/better_command_runner/command_test.dart(7 hunks)packages/cli_tools/test/better_command_runner/default_flags_test.dart(6 hunks)packages/cli_tools/test/better_command_runner/exit_exceptions_test.dart(4 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(8 hunks)packages/cli_tools/test/documentation_generator/generate_markdown_test.dart(5 hunks)packages/cli_tools/test/local_storage_manager/local_storage_manager_test.dart(8 hunks)packages/cli_tools/test/package_version_test.dart(7 hunks)packages/cli_tools/test/prompts/confirm_test.dart(6 hunks)packages/cli_tools/test/prompts/input_test.dart(3 hunks)packages/cli_tools/test/prompts/multiple_select_test.dart(12 hunks)packages/cli_tools/test/prompts/select_test.dart(9 hunks)packages/cli_tools/test/pub_api_client_test.dart(10 hunks)packages/cli_tools/test/std_out_logger_test.dart(12 hunks)packages/cli_tools/test/test_utils/io_helper.dart(1 hunks)packages/cli_tools/test/test_utils/mock_stdin.dart(10 hunks)packages/cli_tools/test/test_utils/mock_stdout.dart(3 hunks)packages/cli_tools/test/test_utils/prompts/option_matcher.dart(5 hunks)packages/config/CHANGELOG.md(1 hunks)packages/config/analysis_options.yaml(1 hunks)packages/config/example/config_file_example.dart(2 hunks)packages/config/example/main.dart(2 hunks)packages/config/lib/config.dart(1 hunks)packages/config/lib/src/config/config.dart(1 hunks)packages/config/lib/src/config/configuration.dart(2 hunks)packages/config/lib/src/config/options.dart(2 hunks)packages/config/pubspec.yaml(2 hunks)packages/config/test/config/args_compatibility/allow_anything_test.dart(1 hunks)packages/config/test/config/args_compatibility/args_test.dart(1 hunks)packages/config/test/config/args_compatibility/command_parse_test.dart(1 hunks)packages/config/test/config/args_compatibility/command_runner_test.dart(1 hunks)packages/config/test/config/args_compatibility/command_test.dart(1 hunks)packages/config/test/config/args_compatibility/parse_performance_test.dart(1 hunks)packages/config/test/config/args_compatibility/parse_test.dart(1 hunks)packages/config/test/config/args_compatibility/test_utils.dart(1 hunks)packages/config/test/config/args_compatibility/trailing_options_test.dart(1 hunks)packages/config/test/config/args_compatibility/usage_test.dart(1 hunks)packages/config/test/config/config_source_test.dart(1 hunks)packages/config/test/config/configuration_test.dart(1 hunks)packages/config/test/config/configuration_type_test.dart(1 hunks)packages/config/test/config/date_parsing_test.dart(1 hunks)packages/config/test/config/duration_parsing_test.dart(1 hunks)packages/config/test/config/file_options_test.dart(1 hunks)pubspec.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/cli_tools/pubspec.yaml
- packages/cli_tools/test/prompts/confirm_test.dart
- packages/config/test/config/configuration_type_test.dart
- packages/config/test/config/args_compatibility/allow_anything_test.dart
- packages/config/test/config/date_parsing_test.dart
- packages/config/test/config/duration_parsing_test.dart
🚧 Files skipped from review as they are similar to previous changes (51)
- .gitignore
- packages/config/test/config/args_compatibility/parse_performance_test.dart
- packages/config/test/config/args_compatibility/test_utils.dart
- packages/config/lib/config.dart
- packages/config/test/config/args_compatibility/parse_test.dart
- packages/config/test/config/args_compatibility/args_test.dart
- packages/config/test/config/config_source_test.dart
- packages/cli_tools/test/prompts/input_test.dart
- packages/config/test/config/args_compatibility/command_runner_test.dart
- packages/cli_tools/test/test_utils/mock_stdout.dart
- packages/config/test/config/configuration_test.dart
- packages/config/pubspec.yaml
- packages/config/lib/src/config/configuration.dart
- packages/cli_tools/lib/logger.dart
- packages/cli_tools/example/main.dart
- melos.yaml
- packages/cli_tools/test/documentation_generator/generate_markdown_test.dart
- pubspec.yaml
- packages/cli_tools/analysis_options.yaml
- packages/cli_tools/lib/src/logger/helpers/ansi_style.dart
- packages/config/test/config/file_options_test.dart
- packages/config/test/config/args_compatibility/trailing_options_test.dart
- packages/cli_tools/test/better_command_runner/exit_exceptions_test.dart
- packages/config/test/config/args_compatibility/command_parse_test.dart
- packages/config/test/config/args_compatibility/usage_test.dart
- packages/config/test/config/args_compatibility/command_test.dart
- packages/cli_tools/test/better_command_runner/analytics_test.dart
- packages/cli_tools/lib/src/logger/loggers/void_logger.dart
- packages/config/analysis_options.yaml
- packages/cli_tools/test/prompts/select_test.dart
- packages/cli_tools/test/test_utils/prompts/option_matcher.dart
- packages/cli_tools/test/better_command_runner/better_command_test.dart
- packages/cli_tools/test/test_utils/io_helper.dart
- packages/cli_tools/lib/src/prompts/confirm.dart
- packages/config/example/config_file_example.dart
- packages/cli_tools/test/prompts/multiple_select_test.dart
- packages/cli_tools/lib/src/prompts/input.dart
- packages/cli_tools/test/pub_api_client_test.dart
- packages/cli_tools/test/better_command_runner/command_test.dart
- packages/cli_tools/lib/src/logger/helpers/progress.dart
- packages/config/lib/src/config/config.dart
- packages/cli_tools/test/package_version_test.dart
- packages/cli_tools/lib/src/documentation_generator/documentation_generator.dart
- packages/cli_tools/example/simple_command_example.dart
- packages/cli_tools/lib/package_version.dart
- packages/cli_tools/lib/src/package_version/pub_api_client.dart
- packages/config/example/main.dart
- packages/cli_tools/test/better_command_runner/logging_test.dart
- packages/cli_tools/lib/src/analytics/analytics.dart
- packages/cli_tools/lib/src/local_storage_manager/local_storage_manager.dart
- packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart
🧰 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: For this project, avoid review comments that simply repeat compile-time errors the Dart analyzer already reports; focus on issues not caught by the analyzer.
Applied to files:
packages/cli_tools/test/better_command_runner/default_flags_test.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/lib/src/config/options.dartpackages/cli_tools/lib/src/better_command_runner/better_command.dart
📚 Learning: 2025-08-15T09:48:28.891Z
Learnt from: christerswahn
PR: serverpod/cli_tools#61
File: packages/cli_tools/lib/src/logger/logger.dart:105-107
Timestamp: 2025-08-15T09:48:28.891Z
Learning: In BoxLogType class in packages/cli_tools/lib/src/logger/logger.dart, the newParagraph parameter in the constructor is intentionally kept for backwards compatibility even though it's not used internally.
Applied to files:
packages/cli_tools/lib/src/logger/logger.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/lib/src/better_command_runner/better_command.dart
🪛 LanguageTool
README.md
[grammar] ~30-~30: There might be a mistake here.
Context: ...tstrap(ormelos bs) to initialize it (this will also run dart pub get`). Ru...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
packages/cli_tools/CHANGELOG.md
3-3: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
packages/config/CHANGELOG.md
3-3: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🔇 Additional comments (30)
packages/cli_tools/test/std_out_logger_test.dart (1)
11-11: Immutability upgrade (final) looks good.Using final for logger instances is consistent with the PR’s lint goals and reduces accidental reassignment in tests.
Also applies to: 95-98
packages/cli_tools/test/local_storage_manager/local_storage_manager_test.dart (2)
3-5: LGTM: import ordering and exception import pathThe imports look consistent and correct with the updated pattern.
85-96: LGTM: happy-path and missing-file cases await Futures correctlyThe success and null-return scenarios correctly await async calls and assert results.
Also applies to: 100-114
packages/config/lib/src/config/options.dart (1)
8-8: LGTM: Import ordering tweak is non-functionalReordering the local imports to include exceptions.dart here is fine and has no behavioral impact.
packages/cli_tools/lib/src/package_version/package_version.dart (1)
22-27: Immutability on function-typed parameters improves API safetyMarking the callbacks as final is a good, non-breaking tightening of the API surface.
packages/cli_tools/lib/src/logger/logger.dart (1)
16-49: LGTM: final parameters on the Logger API are non-breaking and desirableThis tightens parameter mutability without affecting implementers (the final modifier isn’t part of the override contract).
packages/cli_tools/test/test_utils/mock_stdin.dart (1)
21-21: LGTM: setter now correctly has a void bodyThe invalid expression-bodied setter was fixed; this will compile cleanly.
packages/cli_tools/test/better_command_runner/default_flags_test.dart (2)
6-11: LGTM: immutability in test setupConverting runner and callbacks to final improves clarity with no behavioral changes.
16-17: LGTM: args marked final per test caseConsistent with the repo-wide immutability push.
packages/cli_tools/test/better_command_runner/parse_log_level_test.dart (2)
27-38: LGTM: final-qualified parameters in setLogLevel callbackThis maintains readable test intent and doesn’t alter behavior.
78-86: LGTM: immutable args across scenariosNice consistency across quiet/verbose/both flag groups.
packages/cli_tools/lib/src/better_command_runner/better_command.dart (5)
22-22: LGTM! Clean encapsulation of environment variables.The addition of the
envVariablesfield provides good encapsulation and separation of concerns, making environment access explicit.
59-68: LGTM! Constructor improvements enhance immutability and maintainability.The addition of
finalqualifiers to constructor parameters and the new constructor body side effect callingprepareOptionsForParsingare both good improvements that enhance immutability and ensure proper initialization.
74-74: LGTM! Pattern matching improvement adds type safety.The enhanced pattern matching with
case final BetterCommandRunner<O, T> runnerprovides better type safety while maintaining the same functionality.
123-129: LGTM! Configuration resolution updated correctly.The method signature update with
finalparameter and the switch from using theenvparameter toenvVariablesfield is consistent with the architectural changes in this PR.
133-133: LGTM! Method signature consistency.The addition of
finalto thecommandConfigparameter maintains consistency with the immutability improvements across the codebase.packages/cli_tools/lib/src/prompts/select.dart (6)
3-4: LGTM! Import optimization improves dependency clarity.The replacement of the barrel import with specific imports reduces coupling and makes dependencies more explicit, which aligns with good architectural practices.
17-21: LGTM! Parameter immutability improvements.The addition of
finalqualifiers to all function parameters enhances immutability and follows consistent coding practices throughout the PR.Also applies to: 32-36, 45-50
64-66: LGTM! Local variable immutability.The
finalqualifiers on local variables improve code clarity by making the immutable nature of these values explicit.Also applies to: 71-71, 73-74, 79-79
107-111: LGTM! Rendering function improvements.The
finalparameters and local variables in the rendering functions maintain consistency with the immutability theme while preserving functionality.Also applies to: 117-118
178-178: LGTM! State method parameter consistency.The
finalparameters in the_SelectStatemethods maintain consistency with the broader immutability improvements.Also applies to: 184-184
211-215: LGTM! Function parameter consistency.The
finalparameter in theunderlinefunction maintains consistency with the immutability theme across the codebase.packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (8)
69-69: LGTM! Environment variables encapsulation.The addition of the
envVariablesfield provides clean encapsulation of environment variables, making the dependency explicit and testable.
25-27: LGTM! MessageOutput method improvements.The addition of
finalqualifiers to method parameters in theMessageOutputclass enhances immutability and maintains consistency with the broader changes.Also applies to: 31-33, 87-89
139-155: LGTM! Constructor parameter immutability.The addition of
finalqualifiers to all constructor parameters enhances immutability and follows consistent coding practices. The initialization ofenvVariableswith the fallback toPlatform.environmentis well-handled.
180-183: LGTM! Command addition method improvements.The
finalqualifiers on both the method parameter and the loop variable maintain consistency with the immutability theme.
199-201: LGTM! Method signature consistency.The addition of
finalqualifiers to method parameters acrossrun,parse, andrunCommandmethods maintains consistency with the immutability improvements.Also applies to: 223-223, 251-251
265-265: LGTM! Local variable immutability.The
finalqualifiers on local variables improve code clarity by making their immutable nature explicit.Also applies to: 284-284
306-312: LGTM! Configuration resolution updated correctly.The method signature update with
finalparameter and the switch to usingenvVariablesinstead of a directenvparameter aligns with the architectural changes for better environment variable handling.
315-315: LGTM! Static method parameter consistency.The
finalqualifier on theconfigparameter maintains consistency with the immutability improvements across the codebase.
Added pubspec.lock files to .gitignore. Updated code according to standard 'final' use (no behavioral change).
Some docs / README updates.
Introduced melos to manage the repository.
Publish packages:
Summary by CodeRabbit
Chores
Documentation