-
Notifications
You must be signed in to change notification settings - Fork 6k
Add support for dart formatter #57075
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ enum MessageType { | |
| } | ||
|
|
||
| enum FormatCheck { | ||
| dart, | ||
| gn, | ||
| java, | ||
| python, | ||
|
|
@@ -53,6 +54,8 @@ FormatCheck nameToFormatCheck(String name) { | |
| switch (name) { | ||
| case 'clang': | ||
| return FormatCheck.clang; | ||
| case 'dart': | ||
| return FormatCheck.dart; | ||
| case 'gn': | ||
| return FormatCheck.gn; | ||
| case 'java': | ||
|
|
@@ -72,6 +75,8 @@ String formatCheckToName(FormatCheck check) { | |
| switch (check) { | ||
| case FormatCheck.clang: | ||
| return 'C++/ObjC/Shader'; | ||
| case FormatCheck.dart: | ||
| return 'Dart'; | ||
| case FormatCheck.gn: | ||
| return 'GN'; | ||
| case FormatCheck.java: | ||
|
|
@@ -87,8 +92,7 @@ String formatCheckToName(FormatCheck check) { | |
|
|
||
| List<String> formatCheckNames() { | ||
| return FormatCheck.values | ||
| .map<String>((FormatCheck check) => | ||
| check.toString().replaceFirst('$FormatCheck.', '')) | ||
| .map<String>((FormatCheck check) => check.name) | ||
| .toList(); | ||
| } | ||
|
|
||
|
|
@@ -141,6 +145,14 @@ abstract class FormatChecker { | |
| allFiles: allFiles, | ||
| messageCallback: messageCallback, | ||
| ); | ||
| case FormatCheck.dart: | ||
| return DartFormatChecker( | ||
| processManager: processManager, | ||
| baseGitRef: baseGitRef, | ||
| repoDir: repoDir, | ||
| allFiles: allFiles, | ||
| messageCallback: messageCallback, | ||
| ); | ||
| case FormatCheck.gn: | ||
| return GnFormatChecker( | ||
| processManager: processManager, | ||
|
|
@@ -786,6 +798,133 @@ class GnFormatChecker extends FormatChecker { | |
| } | ||
| } | ||
|
|
||
| /// Checks the format of any .dart files using the "dart format" command. | ||
| class DartFormatChecker extends FormatChecker { | ||
| DartFormatChecker({ | ||
| super.processManager, | ||
| required super.baseGitRef, | ||
| required Directory repoDir, | ||
| super.allFiles, | ||
| super.messageCallback, | ||
| }) : super( | ||
| repoDir: repoDir, | ||
| ) { | ||
| // $ENGINE/flutter/third_party/dart/tools/sdks/dart-sdk/bin/dart | ||
| _dartBin = path.join( | ||
| repoDir.absolute.parent.path, | ||
| 'flutter', | ||
| 'third_party', | ||
| 'dart', | ||
| 'tools', | ||
| 'sdks', | ||
| 'dart-sdk', | ||
| 'bin', | ||
| Platform.isWindows ? 'dart.exe' : 'dart', | ||
| ); | ||
| } | ||
|
|
||
| late final String _dartBin; | ||
|
|
||
| @override | ||
| Future<bool> checkFormatting() async { | ||
| message('Checking Dart formatting...'); | ||
| return (await _runDartFormat(fixing: false)) == 0; | ||
| } | ||
|
|
||
| @override | ||
| Future<bool> fixFormatting() async { | ||
| message('Fixing Dart formatting...'); | ||
| await _runDartFormat(fixing: true); | ||
| // The dart formatter shouldn't fail when fixing errors. | ||
| return true; | ||
| } | ||
|
|
||
| Future<int> _runDartFormat({required bool fixing}) async { | ||
| final List<String> filesToCheck = await getFileList(<String>['*.dart']); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a nit: // Todo: we probably should just do this on single list of files rather than forking X processes.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had this modeled after the other language formatters in this file, which all use this pattern. It has the advantage of allowing us to print a nice diff between expected formatting vs. actual formatting. It is probably faster to just pass the list of files directly to the formatter.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Printing a nice diff seems like a good motivation to follow the same pattern.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to print a nice diff. We just need to tell people the files that are failing format checking. They should be running et format before they put the PR up (which in the engine's case is true). |
||
|
|
||
| final List<String> cmd = <String>[ | ||
| _dartBin, | ||
| 'format', | ||
| '--set-exit-if-changed', | ||
| '--show=none', | ||
| if (!fixing) '--output=show', | ||
| if (fixing) '--output=write', | ||
| ]; | ||
| final List<WorkerJob> jobs = <WorkerJob>[]; | ||
| for (final String file in filesToCheck) { | ||
| jobs.add(WorkerJob(<String>[...cmd, file])); | ||
| } | ||
| final ProcessPool dartFmt = ProcessPool( | ||
| processRunner: _processRunner, | ||
| printReport: namedReport('dart format'), | ||
| ); | ||
|
|
||
| Iterable<WorkerJob> incorrect; | ||
| if (!fixing) { | ||
| final Stream<WorkerJob> completedJobs = dartFmt.startWorkers(jobs); | ||
| final List<WorkerJob> diffJobs = <WorkerJob>[]; | ||
| await for (final WorkerJob completedJob in completedJobs) { | ||
| if (completedJob.result.exitCode == 1) { | ||
| diffJobs.add( | ||
| WorkerJob( | ||
| <String>[ | ||
| 'git', | ||
| 'diff', | ||
| '--no-index', | ||
| '--no-color', | ||
| '--ignore-cr-at-eol', | ||
| '--', | ||
| completedJob.command.last, | ||
| '-', | ||
| ], | ||
| stdinRaw: codeUnitsAsStream(completedJob.result.stdoutRaw), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| final ProcessPool diffPool = ProcessPool( | ||
| processRunner: _processRunner, | ||
| printReport: namedReport('diff'), | ||
| ); | ||
| final List<WorkerJob> completedDiffs = await diffPool.runToCompletion(diffJobs); | ||
| incorrect = completedDiffs.where((WorkerJob job) { | ||
| return job.result.exitCode != 0; | ||
| }); | ||
| } else { | ||
| final List<WorkerJob> completedJobs = await dartFmt.runToCompletion(jobs); | ||
| incorrect = completedJobs.where((WorkerJob job) => job.result.exitCode == 1); | ||
| } | ||
|
|
||
| reportDone(); | ||
|
|
||
| if (incorrect.isNotEmpty) { | ||
| final bool plural = incorrect.length > 1; | ||
| if (fixing) { | ||
| message('Fixing ${incorrect.length} dart file${plural ? 's' : ''}' | ||
| ' which ${plural ? 'were' : 'was'} formatted incorrectly.'); | ||
| } else { | ||
| error('Found ${incorrect.length} Dart file${plural ? 's' : ''}' | ||
| ' which ${plural ? 'were' : 'was'} formatted incorrectly.'); | ||
| stdout.writeln('To fix, run `et format` or:'); | ||
| stdout.writeln(); | ||
| stdout.writeln('git apply <<DONE'); | ||
| for (final WorkerJob job in incorrect) { | ||
| stdout.write(job.result.stdout | ||
| .replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}') | ||
| .replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}') | ||
| .replaceFirst(RegExp('\\+Formatted \\d+ files? \\(\\d+ changed\\) in \\d+.\\d+ seconds.\n'), '') | ||
| ); | ||
| } | ||
| stdout.writeln('DONE'); | ||
| stdout.writeln(); | ||
| } | ||
| } else { | ||
| message('All dart files formatted correctly.'); | ||
| } | ||
| return incorrect.length; | ||
| } | ||
| } | ||
|
|
||
| /// Checks the format of any .py files using the "yapf" command. | ||
| class PythonFormatChecker extends FormatChecker { | ||
| PythonFormatChecker({ | ||
|
|
@@ -1144,7 +1283,8 @@ Future<int> main(List<String> arguments) async { | |
| parser.addMultiOption('check', | ||
| abbr: 'c', | ||
| allowed: formatCheckNames(), | ||
| defaultsTo: formatCheckNames(), | ||
| // TODO(goderbauer): Enable dart by default when we turned on the formatter. | ||
| defaultsTo: formatCheckNames()..remove(FormatCheck.dart.name), | ||
| help: 'Specifies which checks will be performed. Defaults to all checks. ' | ||
| 'May be specified more than once to perform multiple types of checks. '); | ||
| parser.addFlag('verbose', help: 'Print verbose output.', defaultsTo: verbose); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is correct, but FYI this is the prebuilt dev channel Dart SDK, and not the prebuilt Dart SDK whose version matches the hash in the DEPS file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zanderso For my own understanding: What's the difference between the two and where is the version of either of these specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The versions are specified in the DEPS file:
The prebuilt dev channel SDK is here:
engine/DEPS
Line 431 in 4d00124
vs.
here we download a prebuilt Dart SDK that matches the version of the Dart SDK source that is pulled into the engine:
engine/DEPS
Line 435 in 4d00124
The version of the Dart SDK that the framework will be using is the second one.