chore: review

Author: marcossevilla

lib/src/commands/create/commands/create_subcommand.dart

@@ -346,9 +346,17 @@ mixin MultiTemplates on CreateSubCommand {
 
     return templates.firstWhere(
       (template) => template.name == templateName,
-      orElse: () => usageException(
-        '"$templateName" is not an allowed value for option "--template".',
-      ),
+      orElse: () {
+        // When the value came from `very_good.yaml` rather than the command
+        // line, point the user at the config key instead of the CLI option so
+        // they debug the right place.
+        final source = argResults.wasParsed('template')
+            ? 'option "--template"'
+            : 'the `create.template` key in `$veryGoodConfigFileName`';
+        usageException(
+          '"$templateName" is not an allowed value for $source.',
+        );
+      },
     );
   }
 }

lib/src/commands/create/create.dart

@@ -93,4 +93,10 @@ class CreateCommand extends Command<int> {
   @override
   String get invocation =>
       'very_good create <subcommand> <project-name> [arguments]';
+
+  /// The names of the subcommands that accept the `--workspace` flag.
+  Set<String> get workspaceSubcommandNames => subcommands.entries
+      .where((entry) => entry.value is Workspace)
+      .map((entry) => entry.key)
+      .toSet();
 }

lib/src/commands/dart/commands/dart_test_command.dart

@@ -283,7 +283,7 @@ class DartTestCommand extends Command<int> {
         help:
             'Whether to collect coverage from imported files only or all '
             'files.',
-        allowed: ['imports', 'all'],
+        allowed: collectCoverageFromAllowedValues,
         defaultsTo: 'imports',
         valueHelp: 'imports|all',
       )

lib/src/commands/packages/commands/check/commands/licenses.dart

@@ -157,12 +157,7 @@ class PackagesCheckLicensesCommand extends Command<int> {
       ..addMultiOption(
         'dependency-type',
         help: 'The type of dependencies to check licenses for.',
-        allowed: [
-          'direct-main',
-          'direct-dev',
-          'direct-overridden',
-          'transitive',
-        ],
+        allowed: dependencyTypeAllowedValues,
         allowedHelp: {
           'direct-main': 'Check for direct main dependencies.',
           'direct-dev': 'Check for direct dev dependencies.',
@@ -183,7 +178,7 @@ class PackagesCheckLicensesCommand extends Command<int> {
       ..addOption(
         'reporter',
         help: 'Lists all licenses.',
-        allowed: ['text', 'csv'],
+        allowed: reporterAllowedValues,
         allowedHelp: {
           'text': 'Lists licenses without a specific format.',
           'csv': 'Lists licenses in a CSV format.',

lib/src/commands/test/test.dart

@@ -333,7 +333,7 @@ class TestCommand extends Command<int> {
         help:
             'Whether to collect coverage from imported files only or all '
             'files.',
-        allowed: ['imports', 'all'],
+        allowed: collectCoverageFromAllowedValues,
         defaultsTo: 'imports',
         valueHelp: 'imports|all',
       )

lib/src/very_good_config/very_good_config.dart

@@ -107,42 +107,23 @@ class VeryGoodConfig extends Equatable {
   /// Loads the closest [VeryGoodConfig] by searching [directory] and each of
   /// its ancestors, from the innermost directory outward.
   ///
-  /// [directory] is resolved to an absolute path before the walk, so a relative
-  /// [directory] is searched relative to the current working directory.
-  ///
-  /// This allows a single repository-wide `very_good.yaml` at the project root
-  /// to apply to commands run from any nested package directory. The first
-  /// configuration file encountered wins; ancestors are not merged.
-  ///
   /// Returns [VeryGoodConfig.empty] when no configuration file is found.
-  /// Throws a [VeryGoodConfigParseException] when the closest file exists but
-  /// cannot be parsed.
-  factory VeryGoodConfig.loadFromClosestAncestor(Directory directory) {
-    var current = directory.absolute;
-    while (true) {
-      final config = _loadFromDirectory(current);
-      if (config != null) return config;
-      final parent = current.parent;
-      if (parent.path == current.path) return VeryGoodConfig.empty;
-      current = parent;
-    }
-  }
-
-  /// Loads the closest [VeryGoodConfig] to [directory].
   ///
   /// On a parse failure, logs a formatted error via [logger] and returns
   /// `null`, signalling that the caller should exit with a config error.
-  ///
-  /// Centralizes the load-and-report contract shared by every command that
-  /// reads `very_good.yaml`, so the error message and exit behavior stay
-  /// consistent.
   static VeryGoodConfig? load(Directory directory, {required Logger logger}) {
     try {
-      return VeryGoodConfig.loadFromClosestAncestor(directory);
+      var current = directory.absolute;
+      while (true) {
+        final config = _loadFromDirectory(current);
+        if (config != null) return config;
+        final parent = current.parent;
+        if (parent.path == current.path) return VeryGoodConfig.empty;
+        current = parent;
+      }
     } on VeryGoodConfigParseException catch (e) {
       logger.err(
-        'Could not read `$veryGoodConfigFileName`.\n'
-        '${e.message}',
+        'Could not read `$veryGoodConfigFileName`.\n${e.message}',
       );
       return null;
     }
@@ -717,35 +698,48 @@ String? _minCoverage(Object? value) {
   return asString;
 }
 
+/// The values accepted by the `collect-coverage-from` option, shared between
+/// the CLI argument parser and the `very_good.yaml` validator so they cannot
+/// drift apart.
+const collectCoverageFromAllowedValues = ['imports', 'all'];
+
+/// The dependency types accepted by `very_good packages check licenses`, shared
+/// between the CLI argument parser and the `very_good.yaml` validator so they
+/// cannot drift apart.
+const dependencyTypeAllowedValues = [
+  'direct-main',
+  'direct-dev',
+  'direct-overridden',
+  'transitive',
+];
+
+/// The values accepted by the license `reporter` option, shared between the
+/// CLI argument parser and the `very_good.yaml` validator so they cannot drift
+/// apart.
+const reporterAllowedValues = ['text', 'csv'];
+
 /// Validates and returns the `collect_coverage_from` value.
 ///
 /// Accepts only `imports` or `all`.
 String? _collectCoverageFrom(Object? value) {
   if (value == null) return null;
-  if (value != 'imports' && value != 'all') {
+  if (!collectCoverageFromAllowedValues.contains(value)) {
     throw FormatException('Expected `imports` or `all` but got `$value`.');
   }
   return value as String;
 }
 
-/// The dependency types accepted by `very_good packages check licenses`.
-const _dependencyTypes = [
-  'direct-main',
-  'direct-dev',
-  'direct-overridden',
-  'transitive',
-];
-
 /// Validates and returns the `dependency_type` value.
 ///
 /// Accepts only the values allowed by the CLI option.
 List<String>? _dependencyType(Object? value) {
   final values = _stringList(value);
   if (values == null) return null;
   for (final value in values) {
-    if (!_dependencyTypes.contains(value)) {
+    if (!dependencyTypeAllowedValues.contains(value)) {
       throw FormatException(
-        'Expected one of ${_dependencyTypes.join(', ')} but got `$value`.',
+        'Expected one of ${dependencyTypeAllowedValues.join(', ')} '
+        'but got `$value`.',
       );
     }
   }
@@ -757,7 +751,7 @@ List<String>? _dependencyType(Object? value) {
 /// Accepts only `text` or `csv`.
 String? _reporter(Object? value) {
   if (value == null) return null;
-  if (value != 'text' && value != 'csv') {
+  if (!reporterAllowedValues.contains(value)) {
     throw FormatException('Expected `text` or `csv` but got `$value`.');
   }
   return value as String;

test/src/commands/create/create_subcommand_test.dart

@@ -1091,7 +1091,8 @@ Run "runner help" to see global options.''';
               isA<UsageException>().having(
                 (e) => e.message,
                 'message',
-                '"unknown" is not an allowed value for option "--template".',
+                '"unknown" is not an allowed value for the `create.template` '
+                    'key in `$veryGoodConfigFileName`.',
               ),
             ),
           );

test/src/very_good_config/very_good_config_test.dart

@@ -497,14 +497,16 @@ packages:
       });
     });
 
-    group('loadFromClosestAncestor', () {
+    group('load', () {
       late Directory tempDir;
       late Directory nestedDir;
+      late Logger logger;
 
       setUp(() {
         tempDir = Directory.systemTemp.createTempSync('very_good_config_');
         nestedDir = Directory(p.join(tempDir.path, 'packages', 'foo'))
           ..createSync(recursive: true);
+        logger = _MockLogger();
       });
 
       tearDown(() {
@@ -513,24 +515,42 @@ packages:
         }
       });
 
+      test('returns the parsed config when very_good.yaml is valid', () {
+        File(p.join(tempDir.path, veryGoodConfigFileName)).writeAsStringSync('''
+packages:
+  get:
+    recursive: true
+''');
+
+        final config = VeryGoodConfig.load(tempDir, logger: logger);
+
+        expect(config, isNotNull);
+        expect(config!.packages.get.recursive, isTrue);
+        verifyNever(() => logger.err(any()));
+      });
+
       test('reads config from the starting directory', () {
         File(p.join(nestedDir.path, veryGoodConfigFileName)).writeAsStringSync(
           '''
 test:
   min_coverage: 80
 ''',
         );
-        final config = VeryGoodConfig.loadFromClosestAncestor(nestedDir);
-        expect(config.test.minCoverage, equals('80'));
+
+        final config = VeryGoodConfig.load(nestedDir, logger: logger);
+
+        expect(config?.test.minCoverage, equals('80'));
       });
 
       test('reads config from an ancestor directory', () {
         File(p.join(tempDir.path, veryGoodConfigFileName)).writeAsStringSync('''
 test:
   min_coverage: 90
 ''');
-        final config = VeryGoodConfig.loadFromClosestAncestor(nestedDir);
-        expect(config.test.minCoverage, equals('90'));
+
+        final config = VeryGoodConfig.load(nestedDir, logger: logger);
+
+        expect(config?.test.minCoverage, equals('90'));
       });
 
       test('prefers the closest config over an ancestor', () {
@@ -544,55 +564,10 @@ test:
   min_coverage: 80
 ''',
         );
-        final config = VeryGoodConfig.loadFromClosestAncestor(nestedDir);
-        expect(config.test.minCoverage, equals('80'));
-      });
 
-      test('returns empty config when no file is found in any ancestor', () {
-        expect(
-          VeryGoodConfig.loadFromClosestAncestor(nestedDir),
-          equals(VeryGoodConfig.empty),
-        );
-      });
+        final config = VeryGoodConfig.load(nestedDir, logger: logger);
 
-      test('rethrows parse exception when the closest file is malformed', () {
-        File(
-          p.join(nestedDir.path, veryGoodConfigFileName),
-        ).writeAsStringSync('- not\n- a\n- map');
-        expect(
-          () => VeryGoodConfig.loadFromClosestAncestor(nestedDir),
-          throwsA(isA<VeryGoodConfigParseException>()),
-        );
-      });
-    });
-
-    group('load', () {
-      late Directory tempDir;
-      late Logger logger;
-
-      setUp(() {
-        tempDir = Directory.systemTemp.createTempSync('very_good_config_');
-        logger = _MockLogger();
-      });
-
-      tearDown(() {
-        if (tempDir.existsSync()) {
-          tempDir.deleteSync(recursive: true);
-        }
-      });
-
-      test('returns the parsed config when very_good.yaml is valid', () {
-        File(p.join(tempDir.path, veryGoodConfigFileName)).writeAsStringSync('''
-packages:
-  get:
-    recursive: true
-''');
-
-        final config = VeryGoodConfig.load(tempDir, logger: logger);
-
-        expect(config, isNotNull);
-        expect(config!.packages.get.recursive, isTrue);
-        verifyNever(() => logger.err(any()));
+        expect(config?.test.minCoverage, equals('80'));
       });
 
       test('returns an empty config when no very_good.yaml exists', () {
@@ -619,6 +594,24 @@ packages:
           ).called(1);
         },
       );
+
+      test(
+        'logs an error and returns null when the closest file is malformed',
+        () {
+          File(
+            p.join(nestedDir.path, veryGoodConfigFileName),
+          ).writeAsStringSync('- not\n- a\n- map');
+
+          final config = VeryGoodConfig.load(nestedDir, logger: logger);
+
+          expect(config, isNull);
+          verify(
+            () => logger.err(
+              any(that: contains('Could not read `very_good.yaml`')),
+            ),
+          ).called(1);
+        },
+      );
     });
 
     test('supports value equality', () {