Skip to content

Commit 7ea7d53

Browse files
stuartmorgan-gfotiDim
authored andcommitted
[flutter_plugin_tools] Improve and test 'format' (flutter#4145)
- Adds unit tests, as there are currently none. - Adds more graceful failure handling. - Adds an internal ignore list to skip files that don't need to be formatted that showed up during local testing. - Adds a note explaining that it's intentially not using the new base command due to performance issues.
1 parent 63bfd1f commit 7ea7d53

File tree

2 files changed

+440
-45
lines changed

2 files changed

+440
-45
lines changed

script/tool/lib/src/format_command.dart

Lines changed: 96 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,11 @@ import 'common/core.dart';
1414
import 'common/plugin_command.dart';
1515
import 'common/process_runner.dart';
1616

17+
const int _exitClangFormatFailed = 3;
18+
const int _exitFlutterFormatFailed = 4;
19+
const int _exitJavaFormatFailed = 5;
20+
const int _exitGitFailed = 6;
21+
1722
final Uri _googleFormatterUrl = Uri.https('github.com',
1823
'/google/google-java-format/releases/download/google-java-format-1.3/google-java-format-1.3-all-deps.jar');
1924

@@ -43,14 +48,18 @@ class FormatCommand extends PluginCommand {
4348
Future<void> run() async {
4449
final String googleFormatterPath = await _getGoogleFormatterPath();
4550

46-
await _formatDart();
47-
await _formatJava(googleFormatterPath);
48-
await _formatCppAndObjectiveC();
51+
// This class is not based on PackageLoopingCommand because running the
52+
// formatters separately for each package is an order of magnitude slower,
53+
// due to the startup overhead of the formatters.
54+
final Iterable<String> files = await _getFilteredFilePaths(getFiles());
55+
await _formatDart(files);
56+
await _formatJava(files, googleFormatterPath);
57+
await _formatCppAndObjectiveC(files);
4958

5059
if (getBoolArg('fail-on-change')) {
5160
final bool modified = await _didModifyAnything();
5261
if (modified) {
53-
throw ToolExit(1);
62+
throw ToolExit(exitCommandFoundErrors);
5463
}
5564
}
5665
}
@@ -60,9 +69,12 @@ class FormatCommand extends PluginCommand {
6069
'git',
6170
<String>['ls-files', '--modified'],
6271
workingDir: packagesDir,
63-
exitOnError: true,
6472
logOnError: true,
6573
);
74+
if (modifiedFiles.exitCode != 0) {
75+
printError('Unable to determine changed files.');
76+
throw ToolExit(_exitGitFailed);
77+
}
6678

6779
print('\n\n');
6880

@@ -79,66 +91,105 @@ class FormatCommand extends PluginCommand {
7991
'pub global run flutter_plugin_tools format" or copy-paste '
8092
'this command into your terminal:');
8193

82-
print('patch -p1 <<DONE');
8394
final io.ProcessResult diff = await processRunner.run(
8495
'git',
8596
<String>['diff'],
8697
workingDir: packagesDir,
87-
exitOnError: true,
8898
logOnError: true,
8999
);
100+
if (diff.exitCode != 0) {
101+
printError('Unable to determine diff.');
102+
throw ToolExit(_exitGitFailed);
103+
}
104+
print('patch -p1 <<DONE');
90105
print(diff.stdout);
91106
print('DONE');
92107
return true;
93108
}
94109

95-
Future<void> _formatCppAndObjectiveC() async {
96-
print('Formatting all .cc, .cpp, .mm, .m, and .h files...');
97-
final Iterable<String> allFiles = <String>[
98-
...await _getFilesWithExtension('.h'),
99-
...await _getFilesWithExtension('.m'),
100-
...await _getFilesWithExtension('.mm'),
101-
...await _getFilesWithExtension('.cc'),
102-
...await _getFilesWithExtension('.cpp'),
103-
];
104-
// Split this into multiple invocations to avoid a
105-
// 'ProcessException: Argument list too long'.
106-
final Iterable<List<String>> batches = partition(allFiles, 100);
107-
for (final List<String> batch in batches) {
108-
await processRunner.runAndStream(getStringArg('clang-format'),
109-
<String>['-i', '--style=Google', ...batch],
110-
workingDir: packagesDir, exitOnError: true);
110+
Future<void> _formatCppAndObjectiveC(Iterable<String> files) async {
111+
final Iterable<String> clangFiles = _getPathsWithExtensions(
112+
files, <String>{'.h', '.m', '.mm', '.cc', '.cpp'});
113+
if (clangFiles.isNotEmpty) {
114+
print('Formatting .cc, .cpp, .h, .m, and .mm files...');
115+
final Iterable<List<String>> batches = partition(clangFiles, 100);
116+
int exitCode = 0;
117+
for (final List<String> batch in batches) {
118+
batch.sort(); // For ease of testing; partition changes the order.
119+
exitCode = await processRunner.runAndStream(
120+
getStringArg('clang-format'),
121+
<String>['-i', '--style=Google', ...batch],
122+
workingDir: packagesDir);
123+
if (exitCode != 0) {
124+
break;
125+
}
126+
}
127+
if (exitCode != 0) {
128+
printError(
129+
'Failed to format C, C++, and Objective-C files: exit code $exitCode.');
130+
throw ToolExit(_exitClangFormatFailed);
131+
}
111132
}
112133
}
113134

114-
Future<void> _formatJava(String googleFormatterPath) async {
115-
print('Formatting all .java files...');
116-
final Iterable<String> javaFiles = await _getFilesWithExtension('.java');
117-
await processRunner.runAndStream('java',
118-
<String>['-jar', googleFormatterPath, '--replace', ...javaFiles],
119-
workingDir: packagesDir, exitOnError: true);
135+
Future<void> _formatJava(
136+
Iterable<String> files, String googleFormatterPath) async {
137+
final Iterable<String> javaFiles =
138+
_getPathsWithExtensions(files, <String>{'.java'});
139+
if (javaFiles.isNotEmpty) {
140+
print('Formatting .java files...');
141+
final int exitCode = await processRunner.runAndStream('java',
142+
<String>['-jar', googleFormatterPath, '--replace', ...javaFiles],
143+
workingDir: packagesDir);
144+
if (exitCode != 0) {
145+
printError('Failed to format Java files: exit code $exitCode.');
146+
throw ToolExit(_exitJavaFormatFailed);
147+
}
148+
}
120149
}
121150

122-
Future<void> _formatDart() async {
123-
// This actually should be fine for non-Flutter Dart projects, no need to
124-
// specifically shell out to dartfmt -w in that case.
125-
print('Formatting all .dart files...');
126-
final Iterable<String> dartFiles = await _getFilesWithExtension('.dart');
127-
if (dartFiles.isEmpty) {
128-
print(
129-
'No .dart files to format. If you set the `--exclude` flag, most likey they were skipped');
130-
} else {
131-
await processRunner.runAndStream(
151+
Future<void> _formatDart(Iterable<String> files) async {
152+
final Iterable<String> dartFiles =
153+
_getPathsWithExtensions(files, <String>{'.dart'});
154+
if (dartFiles.isNotEmpty) {
155+
print('Formatting .dart files...');
156+
// `flutter format` doesn't require the project to actually be a Flutter
157+
// project.
158+
final int exitCode = await processRunner.runAndStream(
132159
'flutter', <String>['format', ...dartFiles],
133-
workingDir: packagesDir, exitOnError: true);
160+
workingDir: packagesDir);
161+
if (exitCode != 0) {
162+
printError('Failed to format Dart files: exit code $exitCode.');
163+
throw ToolExit(_exitFlutterFormatFailed);
164+
}
134165
}
135166
}
136167

137-
Future<List<String>> _getFilesWithExtension(String extension) async =>
138-
getFiles()
139-
.where((File file) => p.extension(file.path) == extension)
140-
.map((File file) => file.path)
141-
.toList();
168+
Future<Iterable<String>> _getFilteredFilePaths(Stream<File> files) async {
169+
// Returns a pattern to check for [directories] as a subset of a file path.
170+
RegExp pathFragmentForDirectories(List<String> directories) {
171+
final String s = p.separator;
172+
return RegExp('(?:^|$s)${p.joinAll(directories)}$s');
173+
}
174+
175+
return files
176+
.map((File file) => file.path)
177+
.where((String path) =>
178+
// Ignore files in build/ directories (e.g., headers of frameworks)
179+
// to avoid useless extra work in local repositories.
180+
!path.contains(
181+
pathFragmentForDirectories(<String>['example', 'build'])) &&
182+
// Ignore files in Pods, which are not part of the repository.
183+
!path.contains(pathFragmentForDirectories(<String>['Pods'])) &&
184+
// Ignore .dart_tool/, which can have various intermediate files.
185+
!path.contains(pathFragmentForDirectories(<String>['.dart_tool'])))
186+
.toList();
187+
}
188+
189+
Iterable<String> _getPathsWithExtensions(
190+
Iterable<String> files, Set<String> extensions) {
191+
return files.where((String path) => extensions.contains(p.extension(path)));
192+
}
142193

143194
Future<String> _getGoogleFormatterPath() async {
144195
final String javaFormatterPath = p.join(

0 commit comments

Comments
 (0)