Skip to content

[tools] Format Dart per-package #8043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 8, 2024
Merged

[tools] Format Dart per-package #8043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 41 additions & 18 deletions script/tool/lib/src/format_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import 'package:meta/meta.dart';

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
Expand Down Expand Up @@ -40,7 +41,7 @@ final Uri _kotlinFormatterUrl = Uri.https('maven.org',
'/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, {
Expand Down Expand Up @@ -85,18 +86,19 @@ class FormatCommand extends PackageCommand {
'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);
}
Expand All @@ -109,7 +111,28 @@ class FormatCommand extends PackageCommand {
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) {
Expand Down Expand Up @@ -291,13 +314,16 @@ class FormatCommand extends PackageCommand {
}
}

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);
Expand Down Expand Up @@ -440,11 +466,8 @@ class FormatCommand extends PackageCommand {
///
/// 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;

Expand All @@ -462,7 +485,7 @@ class FormatCommand extends PackageCommand {
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;
}
Expand Down
50 changes: 16 additions & 34 deletions script/tool/test/format_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,13 @@ void main() {
}

/// 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;
Expand Down Expand Up @@ -100,10 +98,7 @@ void main() {
expect(
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
ProcessCall(
'dart',
<String>['format', ...getPackagesDirRelativePaths(plugin, files)],
packagesDir.path),
ProcessCall('dart', const <String>['format', ...files], plugin.path),
]));
});

Expand Down Expand Up @@ -135,12 +130,7 @@ void main() {
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
ProcessCall(
'dart',
<String>[
'format',
...getPackagesDirRelativePaths(plugin, formattedFiles)
],
packagesDir.path),
'dart', const <String>['format', ...formattedFiles], plugin.path),
]));
});

Expand Down Expand Up @@ -719,12 +709,7 @@ void main() {
],
packagesDir.path),
ProcessCall(
'dart',
<String>[
'format',
...getPackagesDirRelativePaths(plugin, dartFiles)
],
packagesDir.path),
'dart', const <String>['format', ...dartFiles], plugin.path),
ProcessCall(
'java',
<String>[
Expand Down Expand Up @@ -899,11 +884,10 @@ void main() {
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],
Expand All @@ -921,9 +905,9 @@ void main() {
'dart',
<String>[
'format',
'$pluginName\\$extraFile',
extraFile,
],
packagesDir.path),
package.path),
));
});

Expand All @@ -936,8 +920,7 @@ void main() {
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,
Expand All @@ -956,11 +939,10 @@ void main() {
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],
Expand All @@ -978,9 +960,9 @@ void main() {
'dart',
<String>[
'format',
'$pluginName/$extraFile',
extraFile,
],
packagesDir.path),
package.path),
));
});
}