Skip to content

Commit 7deb814

Browse files
rmconsole5-wkrmconsole5-wk
authored
Merge pull request #424 from Workiva/optimize-format-tool
format_tool: optimization pass
2 parents dd56205 + 1b45fb6 commit 7deb814

File tree

3 files changed

+48
-213
lines changed

3 files changed

+48
-213
lines changed

‎lib/src/executable.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ Future<void> runWithConfig(
155155
' but it either does not exist or threw unexpectedly:')
156156
..writeln(' $error')
157157
..writeln()
158-
..writeln('For more info: http://github.com/Workiva/dart_dev#TODO');
158+
..writeln('For more info: https://github.com/Workiva/dart_dev');
159159
exitCode = ExitCode.config.code;
160160
return;
161161
}

‎lib/src/tools/format_tool.dart

Lines changed: 47 additions & 133 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import 'dart:io';
33

44
import 'package:args/args.dart';
55
import 'package:glob/glob.dart';
6-
import 'package:glob/list_local_fs.dart';
76
import 'package:io/ansi.dart';
87
import 'package:io/io.dart' show ExitCode;
98
import 'package:logging/logging.dart';
@@ -140,6 +139,23 @@ class FormatTool extends DevTool {
140139
return exitCode;
141140
}
142141

142+
// Similar to listSync() but does not recurse into hidden directories.
143+
static List<FileSystemEntity> _listSyncWithoutHidden(Directory dir,
144+
{required bool recursive, required bool followLinks}) {
145+
var allEntries = <FileSystemEntity>[];
146+
dir.listSync(recursive: false, followLinks: followLinks).forEach((element) {
147+
final basename = p.basename(element.path);
148+
if (basename.length > 1 && basename.startsWith(".")) return;
149+
150+
allEntries.add(element);
151+
if (element is Directory) {
152+
allEntries.addAll(_listSyncWithoutHidden(element,
153+
recursive: recursive, followLinks: followLinks));
154+
}
155+
});
156+
return allEntries;
157+
}
158+
143159
/// Builds and returns the object that contains:
144160
/// - The file paths
145161
/// - The paths that were excluded by an exclude glob
@@ -151,25 +167,22 @@ class FormatTool extends DevTool {
151167
///
152168
/// By default these globs are assumed to be relative to the current working
153169
/// directory, but that can be overridden via [root] for testing purposes.
154-
///
155-
/// If collapseDirectories is true, directories that contain no exclusions will wind up in the [FormatterInputs],
156-
/// rather than each file in that tree. You may get unexpected results if this and followLinks are both true.
157170
static FormatterInputs getInputs({
158171
List<Glob>? exclude,
159172
bool? expandCwd,
160173
bool? followLinks,
161174
String? root,
162-
bool? collapseDirectories,
175+
@deprecated bool? collapseDirectories,
163176
}) {
177+
if (collapseDirectories != null) {
178+
_log.warning(
179+
'ignoring deprecated option "collapseDirectories": argv limitations are now solved by parallel invocations');
180+
}
181+
164182
expandCwd ??= false;
165183
followLinks ??= false;
166-
collapseDirectories ??= false;
167184

168185
final includedFiles = <String>{};
169-
final excludedFiles = <String>{};
170-
final skippedLinks = <String>{};
171-
final hiddenDirectories = <String>{};
172-
173186
exclude ??= <Glob>[];
174187

175188
if (exclude.isEmpty && !expandCwd) {
@@ -178,135 +191,51 @@ class FormatTool extends DevTool {
178191

179192
final dir = Directory(root ?? '.');
180193

181-
// Use Glob.listSync to get all directories which might include a matching file.
182-
var directoriesWithExcludes = <String>{};
183-
184-
if (collapseDirectories) {
185-
for (var g in exclude) {
186-
List<FileSystemEntity>? matchingPaths;
187-
try {
188-
matchingPaths = g.listSync(followLinks: followLinks);
189-
} on FileSystemException catch (_) {
190-
_log.finer("Glob '$g' did not match any paths.\n");
191-
}
192-
if (matchingPaths != null) {
193-
for (var path in matchingPaths) {
194-
if (path is Directory) {
195-
directoriesWithExcludes.add(path.path);
196-
} else {
197-
directoriesWithExcludes.add(path.parent.path);
198-
}
199-
}
200-
}
201-
}
202-
203-
// This is all the directories that contain a match within them.
204-
_log.finer("Directories with excludes:\n");
205-
for (var dir in directoriesWithExcludes) {
206-
_log.finer(" $dir\n");
207-
}
208-
_log.finer(
209-
"${directoriesWithExcludes.length} directories contain excludes\n");
210-
}
211-
212-
String currentDirectory = p.relative(dir.path, from: dir.path);
213-
bool skipFilesInDirectory = false;
214-
for (final entry
215-
in dir.listSync(recursive: true, followLinks: followLinks)) {
216-
final relative = p.relative(entry.path, from: dir.path);
217-
_log.finest('== Processing relative $relative ==\n');
218-
219-
if (p.isWithin(currentDirectory, relative)) {
220-
if (skipFilesInDirectory) {
221-
_log.finest('skipping child $entry\n');
222-
continue;
223-
}
224-
} else {
225-
// the file/dir in not inside, cancel skipping.
226-
skipFilesInDirectory = false;
227-
}
194+
for (final entry in _listSyncWithoutHidden(dir,
195+
recursive: true, followLinks: followLinks)) {
196+
final filename = p.relative(entry.path, from: dir.path);
197+
_log.finest('== Processing relative $filename ==\n');
228198

229199
if (entry is Link) {
230-
_log.finer('skipping link $relative\n');
231-
skippedLinks.add(relative);
200+
_log.finer('skipping link $filename\n');
232201
continue;
233202
}
234203

235204
if (entry is File && !entry.path.endsWith('.dart')) {
236-
_log.finest('skipping non-dart file $relative\n');
237-
continue;
238-
}
239-
240-
// If the path is in a subdirectory starting with ".", ignore it.
241-
final parts = p.split(relative);
242-
int? hiddenIndex;
243-
for (var i = 0; i < parts.length; i++) {
244-
if (parts[i].startsWith(".")) {
245-
hiddenIndex = i;
246-
break;
247-
}
248-
}
249-
250-
if (hiddenIndex != null) {
251-
final hiddenDirectory = p.joinAll(parts.take(hiddenIndex + 1));
252-
hiddenDirectories.add(hiddenDirectory);
253-
_log.finest('skipping file $relative in hidden dir $hiddenDirectory\n');
254-
if (collapseDirectories) {
255-
currentDirectory = hiddenDirectory;
256-
skipFilesInDirectory = true;
257-
}
205+
_log.finest('skipping non-dart file $filename\n');
258206
continue;
259207
}
260208

261-
if (exclude.any((glob) => glob.matches(relative))) {
262-
_log.finer('excluding $relative\n');
263-
excludedFiles.add(relative);
264-
} else {
265-
if (collapseDirectories && entry is Directory) {
266-
_log.finest('directory: $entry\n');
267-
currentDirectory = relative;
268-
// It seems we can rely on the order of files coming from Directory.listSync.
269-
// If the entry does not contain an excluded file,
270-
// we skip adding any of its children files or directories.
271-
if (directoriesWithExcludes.any(
272-
(directoryWithExclude) =>
273-
p.isWithin(entry.path, directoryWithExclude) ||
274-
p.equals(entry.path, directoryWithExclude),
275-
)) {
276-
_log.finer('$relative has excludes\n');
277-
} else {
278-
skipFilesInDirectory = true;
279-
_log.finer("$relative does not have excludes, skipping children\n");
280-
includedFiles.add(relative);
281-
}
282-
}
283-
284-
if (entry is File && !skipFilesInDirectory) {
285-
_log.finest("adding $relative\n");
286-
includedFiles.add(relative);
287-
}
209+
if (exclude.any((glob) => glob.matches(filename))) {
210+
_log.finer('excluding $filename\n');
211+
} else if (entry is File) {
212+
_log.finest('adding $filename\n');
213+
includedFiles.add(filename);
288214
}
289215
}
290216

291-
_log.finer("excluded ${excludedFiles.length} files\n");
292-
293-
return FormatterInputs(includedFiles,
294-
excludedFiles: excludedFiles,
295-
skippedLinks: skippedLinks,
296-
hiddenDirectories: hiddenDirectories);
217+
return FormatterInputs(includedFiles);
297218
}
298219
}
299220

300221
class FormatterInputs {
301222
FormatterInputs(this.includedFiles,
302-
{this.excludedFiles, this.hiddenDirectories, this.skippedLinks});
223+
{@deprecated this.excludedFiles,
224+
@deprecated this.hiddenDirectories,
225+
@deprecated this.skippedLinks});
303226

227+
final Set<String> includedFiles;
228+
229+
// These fields are deprecated and are likely to be empty, due to
230+
// performance optimizations made in
231+
// https://github.com/Workiva/dart_dev/pull/424
232+
@deprecated
304233
final Set<String>? excludedFiles;
305234

235+
@deprecated
306236
final Set<String>? hiddenDirectories;
307237

308-
final Set<String> includedFiles;
309-
238+
@deprecated
310239
final Set<String>? skippedLinks;
311240
}
312241

@@ -322,6 +251,7 @@ class FormatExecution {
322251
FormatExecution.exitEarly(this.exitCode)
323252
: formatProcess = null,
324253
directiveOrganization = null;
254+
325255
FormatExecution.process(this.formatProcess, [this.directiveOrganization])
326256
: exitCode = null;
327257

@@ -526,7 +456,6 @@ FormatExecution buildExecution(
526456
: FormatTool.getInputs(
527457
exclude: exclude,
528458
root: path,
529-
collapseDirectories: true,
530459
);
531460

532461
if (inputs.includedFiles.isEmpty) {
@@ -536,21 +465,6 @@ FormatExecution buildExecution(
536465
return FormatExecution.exitEarly(ExitCode.config.code);
537466
}
538467

539-
if (inputs.excludedFiles?.isNotEmpty ?? false) {
540-
_log.fine('Excluding these paths from formatting:\n '
541-
'${inputs.excludedFiles!.join('\n ')}');
542-
}
543-
544-
if (inputs.skippedLinks?.isNotEmpty ?? false) {
545-
_log.fine('Excluding these links from formatting:\n '
546-
'${inputs.skippedLinks!.join('\n ')}');
547-
}
548-
549-
if (inputs.hiddenDirectories?.isNotEmpty ?? false) {
550-
_log.fine('Excluding these hidden directories from formatting:\n '
551-
'${inputs.hiddenDirectories!.join('\n ')}');
552-
}
553-
554468
final dartFormatter = buildFormatProcess(formatter);
555469
Iterable<String> args;
556470
if (formatter == Formatter.dartFormat) {

‎test/tools/format_tool_test.dart

Lines changed: 0 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,6 @@ void main() {
4545
test('no excludes', () {
4646
final formatterInputs = FormatTool.getInputs(root: root);
4747
expect(formatterInputs.includedFiles, unorderedEquals({'.'}));
48-
expect(formatterInputs.excludedFiles, null);
49-
expect(formatterInputs.hiddenDirectories, null);
50-
expect(formatterInputs.skippedLinks, null);
5148
});
5249

5350
test('custom excludes', () {
@@ -63,44 +60,6 @@ void main() {
6360
'linked.dart',
6461
p.join('other', 'file.dart'),
6562
}));
66-
67-
expect(formatterInputs.excludedFiles,
68-
unorderedEquals({'should_exclude.dart'}));
69-
expect(formatterInputs.hiddenDirectories,
70-
unorderedEquals({'.dart_tool_test'}));
71-
expect(
72-
formatterInputs.skippedLinks,
73-
unorderedEquals(
74-
{p.join('links', 'lib-link'), p.join('links', 'link.dart')}));
75-
});
76-
77-
test('custom excludes with collapseDirectories', () {
78-
FormatterInputs formatterInputs = FormatTool.getInputs(
79-
exclude: [Glob('*_exclude.dart')],
80-
root: root,
81-
collapseDirectories: true,
82-
);
83-
84-
expect(
85-
formatterInputs.includedFiles,
86-
unorderedEquals({
87-
'file.dart',
88-
'lib',
89-
'linked.dart',
90-
'other',
91-
'links',
92-
}),
93-
);
94-
95-
expect(
96-
formatterInputs.excludedFiles,
97-
unorderedEquals({'should_exclude.dart'}),
98-
);
99-
expect(
100-
formatterInputs.hiddenDirectories,
101-
unorderedEquals({'.dart_tool_test'}),
102-
);
103-
expect(formatterInputs.skippedLinks, isEmpty);
10463
});
10564

10665
test('empty inputs due to excludes config', () async {
@@ -123,7 +82,6 @@ void main() {
12382
p.join('other', 'file.dart'),
12483
'should_exclude.dart',
12584
}));
126-
expect(formatterInputs.excludedFiles, isEmpty);
12785
});
12886

12987
test('followLinks follows linked files and directories', () async {
@@ -136,7 +94,6 @@ void main() {
13694
'not_link.dart',
13795
'link.dart',
13896
}));
139-
expect(formatterInputs.skippedLinks, isEmpty);
14097
});
14198
});
14299
});
@@ -297,42 +254,6 @@ void main() {
297254
expect(execution.exitCode, ExitCode.config.code);
298255
});
299256

300-
test('logs the excluded paths and hidden directories', () async {
301-
var currentLevel = Logger.root.level;
302-
Logger.root.level = Level.FINE;
303-
expect(
304-
Logger.root.onRecord,
305-
emitsInOrder([
306-
fineLogOf(allOf(contains('Excluding these paths'),
307-
contains('should_exclude.dart'))),
308-
fineLogOf(allOf(contains('Excluding these hidden directories'),
309-
contains('.dart_tool_test'))),
310-
]));
311-
312-
buildExecution(DevToolExecutionContext(),
313-
exclude: [Glob('*_exclude.dart')],
314-
path: 'test/tools/fixtures/format/globs');
315-
316-
Logger.root.level = currentLevel;
317-
});
318-
319-
test('logs the skipped links', () async {
320-
var currentLevel = Logger.root.level;
321-
Logger.root.level = Level.FINE;
322-
expect(
323-
Logger.root.onRecord,
324-
emitsInOrder([
325-
fineLogOf(allOf(contains('Excluding these links'),
326-
contains('lib-link'), contains('link.dart'))),
327-
]));
328-
329-
buildExecution(DevToolExecutionContext(),
330-
exclude: [Glob('*_exclude.dart')],
331-
path: 'test/tools/fixtures/format/globs/links');
332-
333-
Logger.root.level = currentLevel;
334-
});
335-
336257
group('returns a FormatExecution', () {
337258
test('', () {
338259
final context = DevToolExecutionContext();

0 commit comments

Comments
 (0)