Skip to content

Commit 659d47b

Browse files
chunhtaichunhtai
committed
addressing comments
1 parent 4385634 commit 659d47b

File tree

6 files changed

+160
-149
lines changed

6 files changed

+160
-149
lines changed

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

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -255,23 +255,18 @@ class RepositoryPackage {
255255
);
256256
}
257257

258-
final List<File> allFiles = pendingChangelogsDir
258+
final List<File> pendingChangelogFiles = pendingChangelogsDir
259259
.listSync()
260260
.whereType<File>()
261+
.where((File file) => !PendingChangelogEntry.isTemplate(file))
261262
.toList();
262-
263-
final pendingChangelogFiles = <File>[];
264-
for (final file in allFiles) {
265-
final String basename = p.basename(file.path);
266-
if (basename.endsWith('.yaml')) {
267-
if (!PendingChangelogEntry.isTemplate(file)) {
268-
pendingChangelogFiles.add(file);
269-
}
270-
} else {
271-
throw FormatException(
272-
'Found non-YAML file in pending_changelogs: ${file.path}',
273-
);
274-
}
263+
final Iterable<File> unexpectedFiles = pendingChangelogFiles.where(
264+
(File file) => !file.basename.endsWith('.yaml'),
265+
);
266+
if (unexpectedFiles.isNotEmpty) {
267+
throw FormatException(
268+
'Found non-YAML file(s) in pending_changelogs: ${unexpectedFiles.map((file) => file.path).join(',')}',
269+
);
275270
}
276271

277272
for (final file in pendingChangelogFiles) {

‎script/tool/lib/src/repo_package_info_check_command.dart‎

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -142,33 +142,13 @@ class RepoPackageInfoCheckCommand extends PackageLoopingCommand {
142142
}
143143

144144
// The content of ci_config.yaml must be valid if there is one.
145-
CIConfig? config;
146145
try {
147-
config = package.parseCIConfig();
146+
package.parseCIConfig();
148147
} on FormatException catch (e) {
149148
printError('$indentation${e.message}');
150149
errors.add(e.message);
151150
}
152151

153-
// All packages with batch release enabled should have valid pending changelogs.
154-
if (config?.isBatchRelease ?? false) {
155-
try {
156-
package.getPendingChangelogs();
157-
} on FormatException catch (e) {
158-
printError('$indentation${e.message}');
159-
errors.add(e.message);
160-
}
161-
} else {
162-
if (package.pendingChangelogsDirectory.existsSync()) {
163-
printError(
164-
'${indentation}Package does not use batch release but has pending changelogs.',
165-
);
166-
errors.add(
167-
'Package does not use batch release but has pending changelogs.',
168-
);
169-
}
170-
}
171-
172152
// All published packages should have a README.md entry.
173153
if (package.isPublishable()) {
174154
errors.addAll(_validateRootReadme(package));

‎script/tool/lib/src/version_check_command.dart‎

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -220,20 +220,40 @@ class VersionCheckCommand extends PackageLoopingCommand {
220220
final errors = <String>[];
221221

222222
final CIConfig? ciConfig = package.parseCIConfig();
223+
final bool usesBatchRelease = ciConfig?.isBatchRelease ?? false;
224+
225+
// All packages with batch release enabled should have valid pending changelogs.
226+
if (usesBatchRelease) {
227+
try {
228+
package.getPendingChangelogs();
229+
} on FormatException catch (e) {
230+
printError('$indentation${e.message}');
231+
errors.add(e.message);
232+
}
233+
} else {
234+
if (package.pendingChangelogsDirectory.existsSync()) {
235+
printError(
236+
'${indentation}Package does not use batch release but has pending changelogs.',
237+
);
238+
errors.add(
239+
'Package does not use batch release but has pending changelogs.',
240+
);
241+
}
242+
}
243+
223244
final _CurrentVersionState versionState = await _getVersionState(
224245
package,
225246
pubspec: pubspec,
226247
);
227-
final bool usesBatchRelease = ciConfig?.isBatchRelease ?? false;
228248
// PR with post release label is going to sync changelog.md and pubspec.yaml
229-
// change back to main branch. Proceed with ragular version check.
249+
// change back to main branch. Proceed with regular version check.
230250
final bool hasPostReleaseLabel = _prLabels.contains(
231251
'post-release-${pubspec.name}',
232252
);
233253
bool versionChanged;
234254

235255
if (usesBatchRelease && !hasPostReleaseLabel) {
236-
versionChanged = await _validateBatchRelease(
256+
versionChanged = await _validatePendingChangeForBatchReleasePackage(
237257
package: package,
238258
changedFiles: changedFiles,
239259
errors: errors,
@@ -646,25 +666,23 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
646666
} else {
647667
missingChangelogChange = true;
648668
final CIConfig? config = package.parseCIConfig();
649-
if (config?.isBatchRelease ?? false) {
650-
printError(
651-
'No new changelog files found in the pending_changelogs folder.\n'
669+
const useOverrideChangelogLabel =
652670
'If this PR needs an exemption from the standard policy of listing '
653671
'all changes in the CHANGELOG,\n'
654672
'comment in the PR to explain why the PR is exempt, and add (or '
655673
'ask your reviewer to add) the\n'
656-
'"$_missingChangelogChangeOverrideLabel" label.\n'
674+
'"$_missingChangelogChangeOverrideLabel" label.\n';
675+
if (config?.isBatchRelease ?? false) {
676+
printError(
677+
'No new changelog files found in the pending_changelogs folder.\n'
678+
'$useOverrideChangelogLabel'
657679
'Otherwise, please add a changelog entry with version:skip in the pending_changelogs folder as described in '
658680
'the contributing guide.\n',
659681
);
660682
} else {
661683
printError(
662684
'No CHANGELOG change found.\n'
663-
'If this PR needs an exemption from the standard policy of listing '
664-
'all changes in the CHANGELOG,\n'
665-
'comment in the PR to explain why the PR is exempt, and add (or '
666-
'ask your reviewer to add) the\n'
667-
'"$_missingChangelogChangeOverrideLabel" label.\n'
685+
'$useOverrideChangelogLabel'
668686
'Otherwise, please add a NEXT entry in the CHANGELOG as described in '
669687
'the contributing guide.\n',
670688
);
@@ -691,7 +709,10 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
691709
return null;
692710
}
693711

694-
Future<bool> _validateBatchRelease({
712+
/// Validates that the pending changelog files are correct for a batch release.
713+
///
714+
/// This should only be called for package that uses batch release.
715+
Future<bool> _validatePendingChangeForBatchReleasePackage({
695716
required RepositoryPackage package,
696717
required List<String> changedFiles,
697718
required List<String> errors,

‎script/tool/test/common/repository_package_test.dart‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,5 +380,27 @@ version: skip
380380
expect(changelogs, hasLength(1));
381381
expect(changelogs[0].changelog, 'A');
382382
});
383+
384+
test('returns an error for non-YAML files', () {
385+
final RepositoryPackage package = createFakePackage(
386+
'a_package',
387+
packagesDir,
388+
);
389+
package.pendingChangelogsDirectory.createSync();
390+
package.pendingChangelogsDirectory
391+
.childFile('readme.txt')
392+
.writeAsStringSync('This is not a YAML file.');
393+
394+
expect(
395+
() => package.getPendingChangelogs(),
396+
throwsA(
397+
isA<FormatException>().having(
398+
(FormatException e) => e.message,
399+
'message',
400+
contains('Found non-YAML file(s) in pending_changelogs'),
401+
),
402+
),
403+
);
404+
});
383405
});
384406
}

‎script/tool/test/repo_package_info_check_command_test.dart‎

Lines changed: 0 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -668,106 +668,5 @@ release:
668668
]),
669669
);
670670
});
671-
672-
test(
673-
'fails for batch release package missing pending_changelogs',
674-
() async {
675-
final RepositoryPackage package = createFakePackage(
676-
'a_package',
677-
packagesDir,
678-
);
679-
680-
root.childFile('README.md').writeAsStringSync('''
681-
${readmeTableHeader()}
682-
${readmeTableEntry('a_package')}
683-
''');
684-
writeAutoLabelerYaml(<RepositoryPackage>[package]);
685-
writeCodeOwners(<RepositoryPackage>[package]);
686-
package.ciConfigFile.writeAsStringSync('''
687-
release:
688-
batch: true
689-
''');
690-
691-
Error? commandError;
692-
final List<String> output = await runCapturingPrint(
693-
runner,
694-
<String>['repo-package-info-check'],
695-
errorHandler: (Error e) {
696-
commandError = e;
697-
},
698-
);
699-
700-
expect(commandError, isA<ToolExit>());
701-
expect(
702-
output,
703-
containsAllInOrder(<Matcher>[
704-
contains('No pending_changelogs folder found for a_package.'),
705-
]),
706-
);
707-
},
708-
);
709-
710-
test('passes for batch release package with pending_changelogs', () async {
711-
final RepositoryPackage package = createFakePackage(
712-
'a_package',
713-
packagesDir,
714-
);
715-
716-
root.childFile('README.md').writeAsStringSync('''
717-
${readmeTableHeader()}
718-
${readmeTableEntry('a_package')}
719-
''');
720-
writeAutoLabelerYaml(<RepositoryPackage>[package]);
721-
writeCodeOwners(<RepositoryPackage>[package]);
722-
package.ciConfigFile.writeAsStringSync('''
723-
release:
724-
batch: true
725-
''');
726-
package.pendingChangelogsDirectory.createSync();
727-
728-
final List<String> output = await runCapturingPrint(runner, <String>[
729-
'repo-package-info-check',
730-
]);
731-
732-
expect(output, containsAll(<Matcher>[contains('No issues found!')]));
733-
});
734-
735-
test(
736-
'fails for non-batch release package with pending_changelogs',
737-
() async {
738-
final RepositoryPackage package = createFakePackage(
739-
'a_package',
740-
packagesDir,
741-
);
742-
743-
root.childFile('README.md').writeAsStringSync('''
744-
${readmeTableHeader()}
745-
${readmeTableEntry('a_package')}
746-
''');
747-
writeAutoLabelerYaml(<RepositoryPackage>[package]);
748-
writeCodeOwners(<RepositoryPackage>[package]);
749-
// No ci_config.yaml means batch release is false by default.
750-
package.pendingChangelogsDirectory.createSync();
751-
752-
Error? commandError;
753-
final List<String> output = await runCapturingPrint(
754-
runner,
755-
<String>['repo-package-info-check'],
756-
errorHandler: (Error e) {
757-
commandError = e;
758-
},
759-
);
760-
761-
expect(commandError, isA<ToolExit>());
762-
expect(
763-
output,
764-
containsAllInOrder(<Matcher>[
765-
contains(
766-
'Package does not use batch release but has pending changelogs.',
767-
),
768-
]),
769-
);
770-
},
771-
);
772671
});
773672
}

‎script/tool/test/version_check_command_test.dart‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1896,6 +1896,100 @@ ${indentation}HTTP response: null
18961896
});
18971897
});
18981898
group('batch release', () {
1899+
test(
1900+
'fails for batch release package missing pending_changelogs',
1901+
() async {
1902+
final RepositoryPackage package = createFakePlugin(
1903+
'a_package',
1904+
packagesDir,
1905+
);
1906+
package.ciConfigFile.writeAsStringSync('''
1907+
release:
1908+
batch: true
1909+
''');
1910+
1911+
Error? commandError;
1912+
final List<String> output = await runCapturingPrint(
1913+
runner,
1914+
<String>['version-check', '--base-sha=main'],
1915+
errorHandler: (Error e) {
1916+
commandError = e;
1917+
},
1918+
);
1919+
1920+
expect(commandError, isA<ToolExit>());
1921+
expect(
1922+
output,
1923+
containsAllInOrder(<Matcher>[
1924+
contains('No pending_changelogs folder found for a_package.'),
1925+
]),
1926+
);
1927+
},
1928+
);
1929+
1930+
test(
1931+
'passes for batch release package with pending_changelogs',
1932+
() async {
1933+
final RepositoryPackage package = createFakePlugin(
1934+
'a_package',
1935+
packagesDir,
1936+
);
1937+
package.ciConfigFile.writeAsStringSync('''
1938+
release:
1939+
batch: true
1940+
''');
1941+
package.pendingChangelogsDirectory.createSync();
1942+
1943+
gitProcessRunner.mockProcessesForExecutable['git-show'] =
1944+
<FakeProcessInfo>[
1945+
FakeProcessInfo(MockProcess(stdout: 'version: 0.0.1')),
1946+
];
1947+
1948+
final List<String> output = await runCapturingPrint(runner, <String>[
1949+
'version-check',
1950+
'--base-sha=main',
1951+
]);
1952+
1953+
expect(
1954+
output,
1955+
containsAllInOrder(<Matcher>[
1956+
contains('Running for a_package'),
1957+
contains('No issues found!'),
1958+
]),
1959+
);
1960+
},
1961+
);
1962+
1963+
test(
1964+
'fails for non-batch release package with pending_changelogs',
1965+
() async {
1966+
final RepositoryPackage package = createFakePlugin(
1967+
'a_package',
1968+
packagesDir,
1969+
);
1970+
// No ci_config.yaml means batch release is false by default.
1971+
package.pendingChangelogsDirectory.createSync();
1972+
1973+
Error? commandError;
1974+
final List<String> output = await runCapturingPrint(
1975+
runner,
1976+
<String>['version-check', '--base-sha=main'],
1977+
errorHandler: (Error e) {
1978+
commandError = e;
1979+
},
1980+
);
1981+
1982+
expect(commandError, isA<ToolExit>());
1983+
expect(
1984+
output,
1985+
containsAllInOrder(<Matcher>[
1986+
contains(
1987+
'Package does not use batch release but has pending changelogs.',
1988+
),
1989+
]),
1990+
);
1991+
},
1992+
);
18991993
test(
19001994
'ignores changelog and pubspec yaml version modifications check with post-release label',
19011995
() async {

0 commit comments

Comments
 (0)