update

Author: chunhtai

script/tool/lib/src/branch_for_batch_release_command.dart

@@ -65,14 +65,15 @@ class BranchForBatchReleaseCommand extends PackageCommand {
     final GitDir repository = await gitDir;
 
     print('Parsing package "${package.displayName}"...');
-    final PendingChangelogs pendingChangelogs = package.getPendingChangelogs();
-    if (pendingChangelogs.errors.isNotEmpty) {
-      printError(
-          'Failed to read pending changelogs for ${package.displayName}:');
-      pendingChangelogs.errors.forEach(printError);
+    List<PendingChangelogEntry> pendingChangelogs = <PendingChangelogEntry>[];
+    try {
+      pendingChangelogs = package.getPendingChangelogs();
+    } on FormatException catch (e) {
+      printError('Failed to parse pending changelogs: ${e.message}');
       throw ToolExit(_kExitPackageMalformed);
     }
-    if (pendingChangelogs.entries.isEmpty) {
+
+    if (pendingChangelogs.isEmpty) {
       print('No pending changelogs found for ${package.displayName}.');
       return;
     }
@@ -85,7 +86,7 @@ class BranchForBatchReleaseCommand extends PackageCommand {
       throw ToolExit(_kExitPackageMalformed);
     }
     final _ReleaseInfo releaseInfo =
-        _getReleaseInfo(pendingChangelogs.entries, pubspec.version!);
+        _getReleaseInfo(pendingChangelogs, pubspec.version!);
 
     if (releaseInfo.newVersion == null) {
       print('No version change specified in pending changelogs for '
@@ -97,7 +98,7 @@ class BranchForBatchReleaseCommand extends PackageCommand {
       git: repository,
       package: package,
       branchName: branchName,
-      pendingChangelogFiles: pendingChangelogs.entries
+      pendingChangelogFiles: pendingChangelogs
           .map<File>((PendingChangelogEntry e) => e.file)
           .toList(),
       releaseInfo: releaseInfo,

script/tool/lib/src/common/repository_package.dart

@@ -240,14 +240,13 @@ class RepositoryPackage {
   ///
   /// Returns the parsed changelog entries for the package, and any errors
   /// that occurred trying to read them.
-  PendingChangelogs getPendingChangelogs() {
+  List<PendingChangelogEntry> getPendingChangelogs() {
     final List<PendingChangelogEntry> entries = <PendingChangelogEntry>[];
-    final List<String> errors = <String>[];
 
     final Directory pendingChangelogsDir = pendingChangelogsDirectory;
     if (!pendingChangelogsDir.existsSync()) {
-      errors.add('No pending_changelogs folder found for $displayName.');
-      return PendingChangelogs(entries, errors);
+      throw FormatException(
+          'No pending_changelogs folder found for $displayName.');
     }
 
     final List<File> allFiles =
@@ -261,25 +260,27 @@ class RepositoryPackage {
           pendingChangelogFiles.add(file);
         }
       } else {
-        errors.add('Found non-YAML file in pending_changelogs: ${file.path}');
+        throw FormatException(
+            'Found non-YAML file in pending_changelogs: ${file.path}');
       }
     }
 
     for (final File file in pendingChangelogFiles) {
       try {
         entries.add(PendingChangelogEntry.parse(file.readAsStringSync(), file));
       } on FormatException catch (e) {
-        errors.add('Malformed pending changelog file: ${file.path}\n$e');
+        throw FormatException(
+            'Malformed pending changelog file: ${file.path}\n$e');
       }
     }
-    return PendingChangelogs(entries, errors);
+    return entries;
   }
 }
 
 /// A class representing the parsed content of a `ci_config.yaml` file.
 class CiConfig {
   /// Creates a [CiConfig] from a parsed YAML map.
-  CiConfig._(this._yaml, this.errors);
+  CiConfig._(this.isBatchRelease);
 
   /// Parses a [CiConfig] from a YAML string.
   factory CiConfig.parse(String yaml) {
@@ -288,10 +289,15 @@ class CiConfig {
       throw const FormatException('Root of ci_config.yaml must be a map.');
     }
 
-    final List<String> errors =
-        _checkCiConfigEntries(loaded, syntax: _validCiConfigSyntax);
+    _checkCiConfigEntries(loaded, syntax: _validCiConfigSyntax);
+
+    bool isBatchRelease = false;
+    final Object? release = loaded['release'];
+    if (release is Map) {
+      isBatchRelease = release['batch'] == true;
+    }
 
-    return CiConfig._(loaded, errors);
+    return CiConfig._(isBatchRelease);
   }
 
   static const Map<String, Object?> _validCiConfigSyntax = <String, Object?>{
@@ -300,26 +306,16 @@ class CiConfig {
     },
   };
 
-  final YamlMap _yaml;
+  /// Returns true if the package is configured for batch release.
+  final bool isBatchRelease;
 
-  /// A list of validation errors found in the config file.
-  final List<String> errors;
 
-  /// Returns true if the package is configured for batch release.
-  bool get isBatchRelease {
-    final Object? release = _yaml['release'];
-    if (release is! Map) {
-      return false;
-    }
-    return release['batch'] == true;
-  }
 
-  static List<String> _checkCiConfigEntries(YamlMap config,
+  static void _checkCiConfigEntries(YamlMap config,
       {required Map<String, Object?> syntax, String configPrefix = ''}) {
-    final List<String> errors = <String>[];
     for (final MapEntry<Object?, Object?> entry in config.entries) {
       if (!syntax.containsKey(entry.key)) {
-        errors.add(
+        throw FormatException(
             'Unknown key `${entry.key}` in config${_formatConfigPrefix(configPrefix)}, the possible keys are ${syntax.keys.toList()}');
       } else {
         final Object syntaxValue = syntax[entry.key]!;
@@ -328,20 +324,19 @@ class CiConfig {
             : '$configPrefix.${entry.key}';
         if (syntaxValue is Set) {
           if (!syntaxValue.contains(entry.value)) {
-            errors.add(
+            throw FormatException(
                 'Invalid value `${entry.value}` for key${_formatConfigPrefix(newConfigPrefix)}, the possible values are ${syntaxValue.toList()}');
           }
         } else if (entry.value is! YamlMap) {
-          errors.add(
+          throw FormatException(
               'Invalid value `${entry.value}` for key${_formatConfigPrefix(newConfigPrefix)}, the value must be a map');
         } else {
-          errors.addAll(_checkCiConfigEntries(entry.value! as YamlMap,
+          _checkCiConfigEntries(entry.value! as YamlMap,
               syntax: syntaxValue as Map<String, Object?>,
-              configPrefix: newConfigPrefix));
+              configPrefix: newConfigPrefix);
         }
       }
     }
-    return errors;
   }
 
   static String _formatConfigPrefix(String configPrefix) =>
@@ -412,14 +407,4 @@ class PendingChangelogEntry {
   final File file;
 }
 
-/// A data class for pending changelogs and any errors found while parsing them.
-class PendingChangelogs {
-  /// Creates a new instance.
-  PendingChangelogs(this.entries, this.errors);
 
-  /// The parsed pending changelog entries.
-  final List<PendingChangelogEntry> entries;
-
-  /// A list of errors found while parsing changelogs.
-  final List<String> errors;
-}

script/tool/lib/src/repo_package_info_check_command.dart

@@ -138,17 +138,22 @@ class RepoPackageInfoCheckCommand extends PackageLoopingCommand {
     }
 
     // The content of ci_config.yaml must be valid if there is one.
-    final CiConfig? ciConfig = package.parseCiConfig();
-    if (ciConfig != null) {
-      for (final String error in ciConfig.errors) {
-        printError('$indentation$error');
-      }
-      errors.addAll(ciConfig.errors);
+    CiConfig? ciConfig;
+    try {
+      ciConfig = package.parseCiConfig();
+    } on FormatException catch (e) {
+      printError('$indentation${e.message}');
+      errors.add(e.message);
     }
 
     // All packages with batch release enabled should have valid pending changelogs.
     if (ciConfig?.isBatchRelease ?? false) {
-      errors.addAll(package.getPendingChangelogs().errors);
+      try {
+        package.getPendingChangelogs();
+      } on FormatException catch (e) {
+        printError('$indentation${e.message}');
+        errors.add(e.message);
+      }
     }
 
     // All published packages should have a README.md entry.

script/tool/lib/src/version_check_command.dart

@@ -625,9 +625,15 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
   Future<List<String>> _validatePendingChangelogs(
       RepositoryPackage package, List<String> changedPaths) async {
     final List<String> errors = <String>[];
-    final PendingChangelogs allChangelogs = package.getPendingChangelogs();
+    List<PendingChangelogEntry> allChangelogs = <PendingChangelogEntry>[];
+    try {
+      allChangelogs = package.getPendingChangelogs();
+    } on FormatException catch (e) {
+      errors.add(e.message);
+      return errors;
+    }
 
-    final List<PendingChangelogEntry> newEntries = allChangelogs.entries
+    final List<PendingChangelogEntry> newEntries = allChangelogs
         .where((PendingChangelogEntry entry) =>
             changedPaths.contains(entry.file.path))
         .toList();

script/tool/test/common/repository_package_test.dart

@@ -287,11 +287,7 @@ release:
 foo: bar
 ''');
 
-      final CiConfig? config = plugin.parseCiConfig();
-
-      expect(config, isNotNull);
-      expect(config!.errors, hasLength(1));
-      expect(config.errors[0], contains('Unknown key `foo`'));
+      expect(() => plugin.parseCiConfig(), throwsFormatException);
     });
 
     test('reports invalid values', () {
@@ -302,11 +298,7 @@ release:
   batch: not-a-bool
 ''');
 
-      final CiConfig? config = plugin.parseCiConfig();
-
-      expect(config, isNotNull);
-      expect(config!.errors, hasLength(1));
-      expect(config.errors[0], contains('Invalid value `not-a-bool`'));
+      expect(() => plugin.parseCiConfig(), throwsFormatException);
     });
   });
 
@@ -315,22 +307,18 @@ release:
       final RepositoryPackage package =
           createFakePackage('a_package', packagesDir);
 
-      final PendingChangelogs changelogs = package.getPendingChangelogs();
-
-      expect(changelogs.entries, isEmpty);
-      expect(changelogs.errors, hasLength(1));
-      expect(changelogs.errors[0], contains('No pending_changelogs folder'));
+      expect(() => package.getPendingChangelogs(), throwsFormatException);
     });
 
     test('returns empty lists if the directory is empty', () {
       final RepositoryPackage package =
           createFakePackage('a_package', packagesDir);
       package.pendingChangelogsDirectory.createSync();
 
-      final PendingChangelogs changelogs = package.getPendingChangelogs();
+      final List<PendingChangelogEntry> changelogs =
+          package.getPendingChangelogs();
 
-      expect(changelogs.entries, isEmpty);
-      expect(changelogs.errors, isEmpty);
+      expect(changelogs, isEmpty);
     });
 
     test('returns entries for valid files', () {
@@ -350,14 +338,14 @@ changelog: B
 version: minor
 ''');
 
-      final PendingChangelogs changelogs = package.getPendingChangelogs();
+      final List<PendingChangelogEntry> changelogs =
+          package.getPendingChangelogs();
 
-      expect(changelogs.errors, isEmpty);
-      expect(changelogs.entries, hasLength(2));
-      expect(changelogs.entries[0].changelog, 'A');
-      expect(changelogs.entries[0].version, VersionChange.patch);
-      expect(changelogs.entries[1].changelog, 'B');
-      expect(changelogs.entries[1].version, VersionChange.minor);
+      expect(changelogs, hasLength(2));
+      expect(changelogs[0].changelog, 'A');
+      expect(changelogs[0].version, VersionChange.patch);
+      expect(changelogs[1].changelog, 'B');
+      expect(changelogs[1].version, VersionChange.minor);
     });
 
     test('returns an error for a malformed file', () {
@@ -368,12 +356,7 @@ version: minor
           package.pendingChangelogsDirectory.childFile('a.yaml');
       changelogFile.writeAsStringSync('not yaml');
 
-      final PendingChangelogs changelogs = package.getPendingChangelogs();
-
-      expect(changelogs.entries, isEmpty);
-      expect(changelogs.errors, hasLength(1));
-      expect(
-          changelogs.errors[0], contains('Malformed pending changelog file'));
+      expect(() => package.getPendingChangelogs(), throwsFormatException);
     });
 
     test('ignores template.yaml', () {
@@ -393,11 +376,11 @@ changelog: TEMPLATE
 version: skip
 ''');
 
-      final PendingChangelogs changelogs = package.getPendingChangelogs();
+      final List<PendingChangelogEntry> changelogs =
+          package.getPendingChangelogs();
 
-      expect(changelogs.errors, isEmpty);
-      expect(changelogs.entries, hasLength(1));
-      expect(changelogs.entries[0].changelog, 'A');
+      expect(changelogs, hasLength(1));
+      expect(changelogs[0].changelog, 'A');
     });
 
     test('returns an error for a non-yaml file', () {
@@ -414,11 +397,7 @@ version: patch
           .childFile('a.txt')
           .writeAsStringSync('text');
 
-      final PendingChangelogs changelogs = package.getPendingChangelogs();
-
-      expect(changelogs.entries, hasLength(1));
-      expect(changelogs.errors, hasLength(1));
-      expect(changelogs.errors[0], contains('Found non-YAML file'));
+      expect(() => package.getPendingChangelogs(), throwsFormatException);
     });
   });
 }

script/tool/test/repo_package_info_check_command_test.dart

@@ -604,7 +604,7 @@ release:
         output,
         containsAllInOrder(<Matcher>[
           contains(
-              'Packages with batch releases enabled must have a "pending_changelogs" directory.'),
+              'No pending_changelogs folder found for a_package.'),
         ]),
       );
     });

script/tool/test/version_check_command_test.dart

@@ -369,7 +369,7 @@ release:
       expect(
           output,
           containsAllInOrder(<Matcher>[
-            contains('Missing CHANGELOG file'),
+            contains('No pending_changelogs folder found for package'),
           ]));
     });