[tools] Format Dart per-package (#8043)
Instead of running Dart formatting on the whole repo at once, run it per
package, from the package's directory. This is slower, but necessary
since the new formatter behaves differently depending on the package's
min SDK version.
diff --git a/script/tool/lib/src/format_command.dart b/script/tool/lib/src/format_command.dart
index b5e6086..f93f221 100644
--- a/script/tool/lib/src/format_command.dart
+++ b/script/tool/lib/src/format_command.dart
@@ -11,7 +11,8 @@
import 'common/core.dart';
import 'common/output_utils.dart';
-import 'common/package_command.dart';
+import 'common/package_looping_command.dart';
+import 'common/repository_package.dart';
/// In theory this should be 8191, but in practice that was still resulting in
/// "The input line is too long" errors. This was chosen as a value that worked
@@ -40,7 +41,7 @@
'/maven2/com/facebook/ktfmt/0.46/ktfmt-0.46-jar-with-dependencies.jar');
/// A command to format all package code.
-class FormatCommand extends PackageCommand {
+class FormatCommand extends PackageLoopingCommand {
/// Creates an instance of the format command.
FormatCommand(
super.packagesDir, {
@@ -85,18 +86,19 @@
'to be in your path.';
@override
- Future<void> run() async {
+ Future<void> initializeRun() async {
final String javaFormatterPath = await _getJavaFormatterPath();
final String kotlinFormatterPath = await _getKotlinFormatterPath();
- // This class is not based on PackageLoopingCommand because running the
- // formatters separately for each package is an order of magnitude slower,
- // due to the startup overhead of the formatters.
+ // All but Dart is formatted here rather than in runForPackage because
+ // running the formatters separately for each package is an order of
+ // magnitude slower, due to the startup overhead of the formatters.
+ //
+ // Dart has to be run per-package because the formatter can have different
+ // behavior based on the package's SDK, which can't be determined if the
+ // formatter isn't running in the context of the package.
final Iterable<String> files =
await _getFilteredFilePaths(getFiles(), relativeTo: packagesDir);
- if (getBoolArg(_dartArg)) {
- await _formatDart(files);
- }
if (getBoolArg(_javaArg)) {
await _formatJava(files, javaFormatterPath);
}
@@ -109,7 +111,28 @@
if (getBoolArg(_swiftArg)) {
await _formatAndLintSwift(files);
}
+ }
+ @override
+ Future<PackageResult> runForPackage(RepositoryPackage package) async {
+ final Iterable<String> files = await _getFilteredFilePaths(
+ getFilesForPackage(package),
+ relativeTo: package.directory,
+ );
+ if (getBoolArg(_dartArg)) {
+ await _formatDart(files, workingDir: package.directory);
+ }
+ // Success or failure is determined overall in completeRun, since most code
+ // isn't being validated per-package, so just always return success at the
+ // package level.
+ // TODO(stuartmorgan): Consider doing _didModifyAnything checks per-package
+ // instead, since the other languages are already formatted by the time
+ // this code is being run.
+ return PackageResult.success();
+ }
+
+ @override
+ Future<void> completeRun() async {
if (getBoolArg(_failonChangeArg)) {
final bool modified = await _didModifyAnything();
if (modified) {
@@ -291,13 +314,16 @@
}
}
- Future<void> _formatDart(Iterable<String> files) async {
+ Future<void> _formatDart(
+ Iterable<String> files, {
+ Directory? workingDir,
+ }) async {
final Iterable<String> dartFiles =
_getPathsWithExtensions(files, <String>{'.dart'});
if (dartFiles.isNotEmpty) {
print('Formatting .dart files...');
- final int exitCode =
- await _runBatched('dart', <String>['format'], files: dartFiles);
+ final int exitCode = await _runBatched('dart', <String>['format'],
+ files: dartFiles, workingDir: workingDir);
if (exitCode != 0) {
printError('Failed to format Dart files: exit code $exitCode.');
throw ToolExit(_exitFlutterFormatFailed);
@@ -440,11 +466,8 @@
///
/// Returns the exit code of the first failure, which stops the run, or 0
/// on success.
- Future<int> _runBatched(
- String command,
- List<String> arguments, {
- required Iterable<String> files,
- }) async {
+ Future<int> _runBatched(String command, List<String> arguments,
+ {required Iterable<String> files, Directory? workingDir}) async {
final int commandLineMax =
platform.isWindows ? windowsCommandLineMax : nonWindowsCommandLineMax;
@@ -462,7 +485,7 @@
batch.sort(); // For ease of testing.
final int exitCode = await processRunner.runAndStream(
command, <String>[...arguments, ...batch],
- workingDir: packagesDir);
+ workingDir: workingDir ?? packagesDir);
if (exitCode != 0) {
return exitCode;
}
diff --git a/script/tool/test/format_command_test.dart b/script/tool/test/format_command_test.dart
index 33f5104..b2c8237 100644
--- a/script/tool/test/format_command_test.dart
+++ b/script/tool/test/format_command_test.dart
@@ -63,15 +63,13 @@
}
/// Returns a list of [count] relative paths to pass to [createFakePlugin]
- /// or [createFakePackage] with name [packageName] such that each path will
- /// be 99 characters long relative to [packagesDir].
+ /// or [createFakePackage] such that each path will be 99 characters long
+ /// relative to the package directory.
///
/// This is for each of testing batching, since it means each file will
/// consume 100 characters of the batch length.
- List<String> get99CharacterPathExtraFiles(String packageName, int count) {
- final int padding = 99 -
- packageName.length -
- 1 - // the path separator after the package name
+ List<String> get99CharacterPathExtraFiles(int count) {
+ const int padding = 99 -
1 - // the path separator after the padding
10; // the file name
const int filenameBase = 10000;
@@ -100,10 +98,7 @@
expect(
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
- ProcessCall(
- 'dart',
- <String>['format', ...getPackagesDirRelativePaths(plugin, files)],
- packagesDir.path),
+ ProcessCall('dart', const <String>['format', ...files], plugin.path),
]));
});
@@ -135,12 +130,7 @@
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
ProcessCall(
- 'dart',
- <String>[
- 'format',
- ...getPackagesDirRelativePaths(plugin, formattedFiles)
- ],
- packagesDir.path),
+ 'dart', const <String>['format', ...formattedFiles], plugin.path),
]));
});
@@ -719,12 +709,7 @@
],
packagesDir.path),
ProcessCall(
- 'dart',
- <String>[
- 'format',
- ...getPackagesDirRelativePaths(plugin, dartFiles)
- ],
- packagesDir.path),
+ 'dart', const <String>['format', ...dartFiles], plugin.path),
ProcessCall(
'java',
<String>[
@@ -899,11 +884,10 @@
const int batchSize = (windowsCommandLineMax ~/ 100) - 1;
// Make the file list one file longer than would fit in the batch.
- final List<String> batch1 =
- get99CharacterPathExtraFiles(pluginName, batchSize + 1);
+ final List<String> batch1 = get99CharacterPathExtraFiles(batchSize + 1);
final String extraFile = batch1.removeLast();
- createFakePlugin(
+ final RepositoryPackage package = createFakePlugin(
pluginName,
packagesDir,
extraFiles: <String>[...batch1, extraFile],
@@ -921,9 +905,9 @@
'dart',
<String>[
'format',
- '$pluginName\\$extraFile',
+ extraFile,
],
- packagesDir.path),
+ package.path),
));
});
@@ -936,8 +920,7 @@
const int batchSize = (windowsCommandLineMax ~/ 100) - 1;
// Make the file list one file longer than would fit in a Windows batch.
- final List<String> batch =
- get99CharacterPathExtraFiles(pluginName, batchSize + 1);
+ final List<String> batch = get99CharacterPathExtraFiles(batchSize + 1);
createFakePlugin(
pluginName,
@@ -956,11 +939,10 @@
const int batchSize = (nonWindowsCommandLineMax ~/ 100) - 1;
// Make the file list one file longer than would fit in the batch.
- final List<String> batch1 =
- get99CharacterPathExtraFiles(pluginName, batchSize + 1);
+ final List<String> batch1 = get99CharacterPathExtraFiles(batchSize + 1);
final String extraFile = batch1.removeLast();
- createFakePlugin(
+ final RepositoryPackage package = createFakePlugin(
pluginName,
packagesDir,
extraFiles: <String>[...batch1, extraFile],
@@ -978,9 +960,9 @@
'dart',
<String>[
'format',
- '$pluginName/$extraFile',
+ extraFile,
],
- packagesDir.path),
+ package.path),
));
});
}
