Skip to content

[ci] Add flags to formatter command to decide which formatters to run #5905

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
auto-submit merged 3 commits into from
Jan 18, 2024
Merged

[ci] Add flags to formatter command to decide which formatters to run #5905

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64cf5a5
[ci] Add flags to formatter command to decide which formatters to run
jmagman Jan 16, 2024
ca0792d
remove platform check
jmagman Jan 16, 2024
e937de9
flag tests
jmagman Jan 17, 2024
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
75 changes: 56 additions & 19 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 @@ -33,6 +33,7 @@ const int _exitGitFailed = 6;
const int _exitDependencyMissing = 7;
const int _exitSwiftFormatFailed = 8;
const int _exitKotlinFormatFailed = 9;
const int _exitUnsupportedPlatform = 10;

final Uri _javaFormatterUrl = Uri.https('github.com',
'/google/google-java-format/releases/download/google-java-format-1.3/google-java-format-1.3-all-deps.jar');
Expand All @@ -48,17 +49,29 @@ class FormatCommand extends PackageCommand {
super.platform,
}) {
argParser.addFlag('fail-on-change', hide: true);
argParser.addOption(_clangFormatArg,
argParser.addFlag(_dartArg, help: 'Format Dart files', defaultsTo: true);
argParser.addFlag(_clangFormatArg,
help: 'Format with "clang-format"', defaultsTo: true);
argParser.addFlag(_kotlinArg,
help: 'Format Kotlin files', defaultsTo: true);
argParser.addFlag(_javaArg, help: 'Format Java files', defaultsTo: true);
argParser.addFlag(_swiftArg, help: 'Format Swift files');
argParser.addOption(_clangFormatPathArg,
defaultsTo: 'clang-format', help: 'Path to "clang-format" executable.');
argParser.addOption(_javaArg,
argParser.addOption(_javaPathArg,
defaultsTo: 'java', help: 'Path to "java" executable.');
argParser.addOption(_swiftFormatArg,
help: 'Path to "swift-format" executable.');
argParser.addOption(_swiftFormatPathArg,
defaultsTo: 'swift-format', help: 'Path to "swift-format" executable.');
}

static const String _dartArg = 'dart';
static const String _clangFormatArg = 'clang-format';
static const String _kotlinArg = 'kotlin';
static const String _javaArg = 'java';
static const String _swiftFormatArg = 'swift-format';
static const String _swiftArg = 'swift';
static const String _clangFormatPathArg = 'clang-format-path';
static const String _javaPathArg = 'java-path';
static const String _swiftFormatPathArg = 'swift-format-path';

@override
final String name = 'format';
Expand All @@ -80,13 +93,25 @@ class FormatCommand extends PackageCommand {
// due to the startup overhead of the formatters.
final Iterable<String> files =
await _getFilteredFilePaths(getFiles(), relativeTo: packagesDir);
await _formatDart(files);
await _formatJava(files, javaFormatterPath);
await _formatKotlin(files, kotlinFormatterPath);
await _formatCppAndObjectiveC(files);
final String? swiftFormat = getNullableStringArg(_swiftFormatArg);
if (swiftFormat != null) {
await _formatSwift(swiftFormat, files);
if (getBoolArg(_dartArg)) {
Copy link
Member Author

@jmagman jmagman Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to add tests for these flags.

await _formatDart(files);
}
if (getBoolArg(_javaArg)) {
await _formatJava(files, javaFormatterPath);
}
if (getBoolArg(_kotlinArg)) {
await _formatKotlin(files, kotlinFormatterPath);
}
if (getBoolArg(_clangFormatArg)) {
await _formatCppAndObjectiveC(files);
}
if (getBoolArg(_swiftArg)) {
/// swift-format can run on Linux, but in CI it is only built for macOS.
if (!platform.isMacOS) {
printError('swift-format is only supported on macOS');
throw ToolExit(_exitUnsupportedPlatform);
}
await _formatSwift(files);
}

if (getBoolArg('fail-on-change')) {
Expand Down Expand Up @@ -158,7 +183,8 @@ class FormatCommand extends PackageCommand {
}
}

Future<void> _formatSwift(String swiftFormat, Iterable<String> files) async {
Future<void> _formatSwift(Iterable<String> files) async {
final String swiftFormat = await _findValidSwiftFormat();
final Iterable<String> swiftFiles =
_getPathsWithExtensions(files, <String>{'.swift'});
if (swiftFiles.isNotEmpty) {
Expand All @@ -173,7 +199,7 @@ class FormatCommand extends PackageCommand {
}

Future<String> _findValidClangFormat() async {
final String clangFormat = getStringArg(_clangFormatArg);
final String clangFormat = getStringArg(_clangFormatPathArg);
if (await _hasDependency(clangFormat)) {
return clangFormat;
}
Expand All @@ -188,19 +214,30 @@ class FormatCommand extends PackageCommand {
}
}
printError('Unable to run "clang-format". Make sure that it is in your '
'path, or provide a full path with --clang-format.');
'path, or provide a full path with --$_clangFormatPathArg.');
throw ToolExit(_exitDependencyMissing);
}

Future<String> _findValidSwiftFormat() async {
final String swiftFormat = getStringArg(_swiftFormatPathArg);
if (await _hasDependency(swiftFormat)) {
return swiftFormat;
}

printError('Unable to run "swift-format". Make sure that it is in your '
'path, or provide a full path with --$_swiftFormatPathArg.');
throw ToolExit(_exitDependencyMissing);
}

Future<void> _formatJava(Iterable<String> files, String formatterPath) async {
final Iterable<String> javaFiles =
_getPathsWithExtensions(files, <String>{'.java'});
if (javaFiles.isNotEmpty) {
final String java = getStringArg('java');
final String java = getStringArg(_javaPathArg);
if (!await _hasDependency(java)) {
printError(
'Unable to run "java". Make sure that it is in your path, or '
'provide a full path with --java.');
'provide a full path with --$_javaPathArg.');
throw ToolExit(_exitDependencyMissing);
}

Expand All @@ -220,11 +257,11 @@ class FormatCommand extends PackageCommand {
final Iterable<String> kotlinFiles =
_getPathsWithExtensions(files, <String>{'.kt'});
if (kotlinFiles.isNotEmpty) {
final String java = getStringArg('java');
final String java = getStringArg(_javaPathArg);
if (!await _hasDependency(java)) {
printError(
'Unable to run "java". Make sure that it is in your path, or '
'provide a full path with --java.');
'provide a full path with --$_javaPathArg.');
throw ToolExit(_exitDependencyMissing);
}

Expand Down
89 changes: 77 additions & 12 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 @@ -17,23 +17,32 @@ import 'util.dart';
void main() {
late FileSystem fileSystem;
late MockPlatform mockPlatform;
late MockPlatform mockMacOSPlatform;
late Directory packagesDir;
late RecordingProcessRunner processRunner;
late FormatCommand analyzeCommand;
late FormatCommand macOSAnalyzeCommand;
late CommandRunner<void> runner;
late CommandRunner<void> macOSRunner;
late String javaFormatPath;
late String kotlinFormatPath;

setUp(() {
fileSystem = MemoryFileSystem();
mockPlatform = MockPlatform();
mockMacOSPlatform = MockPlatform(isMacOS: true);
packagesDir = createPackagesDirectory(fileSystem: fileSystem);
processRunner = RecordingProcessRunner();
analyzeCommand = FormatCommand(
packagesDir,
processRunner: processRunner,
platform: mockPlatform,
);
macOSAnalyzeCommand = FormatCommand(
packagesDir,
processRunner: processRunner,
platform: mockMacOSPlatform,
);

// Create the Java and Kotlin formatter files that the command checks for,
// to avoid a download.
Expand All @@ -48,6 +57,10 @@ void main() {

runner = CommandRunner<void>('format_command', 'Test for format_command');
runner.addCommand(analyzeCommand);

macOSRunner = CommandRunner<void>(
'format_command', 'Test for format_command on macOS');
macOSRunner.addCommand(macOSAnalyzeCommand);
});

/// Returns a modified version of a list of [relativePaths] that are relative
Expand Down Expand Up @@ -220,7 +233,7 @@ void main() {
containsAllInOrder(<Matcher>[
contains(
'Unable to run "java". Make sure that it is in your path, or '
'provide a full path with --java.'),
'provide a full path with --java-path.'),
]));
});

Expand Down Expand Up @@ -261,7 +274,8 @@ void main() {
extraFiles: files,
);

await runCapturingPrint(runner, <String>['format', '--java=/path/to/java']);
await runCapturingPrint(
runner, <String>['format', '--java-path=/path/to/java']);

expect(
processRunner.recordedCalls,
Expand Down Expand Up @@ -332,7 +346,7 @@ void main() {
output,
containsAllInOrder(<Matcher>[
contains('Unable to run "clang-format". Make sure that it is in your '
'path, or provide a full path with --clang-format.'),
'path, or provide a full path with --clang-format-path.'),
]));
});

Expand Down Expand Up @@ -386,8 +400,8 @@ void main() {
extraFiles: files,
);

await runCapturingPrint(
runner, <String>['format', '--clang-format=/path/to/clang-format']);
await runCapturingPrint(runner,
<String>['format', '--clang-format-path=/path/to/clang-format']);

expect(
processRunner.recordedCalls,
Expand Down Expand Up @@ -490,6 +504,21 @@ void main() {
});

group('swift-format', () {
test('fails formatting Swift on non-macOS platforms', () async {
Error? commandError;
final List<String> output = await runCapturingPrint(
runner, <String>['format', '--swift'], errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(<Matcher>[
contains('swift-format is only supported on macOS'),
]));
});

test('formats Swift if --swift-format flag is provided', () async {
const List<String> files = <String>[
'macos/foo.swift',
Expand All @@ -500,20 +529,25 @@ void main() {
extraFiles: files,
);

await runCapturingPrint(
runner, <String>['format', '--swift-format=/path/to/swift-format']);
await runCapturingPrint(macOSRunner, <String>[
'format',
'--swift',
'--swift-format-path=/path/to/swift-format'
]);

expect(
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
const ProcessCall(
'/path/to/swift-format', <String>['--version'], null),
ProcessCall(
'/path/to/swift-format',
<String>['-i', ...getPackagesDirRelativePaths(plugin, files)],
packagesDir.path),
]));
});

test('skips Swift if --swift-format flag is not provided', () async {
test('skips Swift if --swift flag is not provided', () async {
const List<String> files = <String>[
'macos/foo.swift',
];
Expand All @@ -523,11 +557,38 @@ void main() {
extraFiles: files,
);

await runCapturingPrint(runner, <String>['format']);
await runCapturingPrint(macOSRunner, <String>['format']);

expect(processRunner.recordedCalls, orderedEquals(<ProcessCall>[]));
});

test('fails with a clear message if swift-format is not in the path',
() async {
const List<String> files = <String>[
'macos/foo.swift',
];
createFakePlugin('a_plugin', packagesDir, extraFiles: files);

processRunner.mockProcessesForExecutable['swift-format'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(exitCode: 1), <String>['--version']),
];
Error? commandError;
final List<String> output = await runCapturingPrint(
macOSRunner, <String>['format', '--swift'], errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(<Matcher>[
contains(
'Unable to run "swift-format". Make sure that it is in your path, or '
'provide a full path with --swift-format-path.'),
]));
});

test('fails if swift-format fails', () async {
const List<String> files = <String>[
'macos/foo.swift',
Expand All @@ -536,12 +597,16 @@ void main() {

processRunner.mockProcessesForExecutable['swift-format'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(),
<String>['--version']), // check for working swift-format
FakeProcessInfo(MockProcess(exitCode: 1), <String>['-i']),
];
Error? commandError;
final List<String> output = await runCapturingPrint(
runner, <String>['format', '--swift-format=swift-format'],
errorHandler: (Error e) {
final List<String> output = await runCapturingPrint(macOSRunner, <String>[
'format',
'--swift',
'--swift-format-path=swift-format'
], errorHandler: (Error e) {
commandError = e;
});

Expand Down