Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[tool] Add a way to opt a file out of formatting #4292

Merged
merged 1 commit into from
Sep 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion script/tool/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
## NEXT
## 0.7.0

- `native-test` now supports `--linux` for unit tests.
- Formatting now skips Dart files that contain a line that exactly
matches the string `// This file is hand-formatted.`.

## 0.6.0+1

Expand Down
20 changes: 20 additions & 0 deletions script/tool/lib/src/format_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,27 @@ class FormatCommand extends PluginCommand {

final String fromPath = relativeTo.path;

// Dart files are allowed to have a pragma to disable auto-formatting. This
// was added because Hixie hurts when dealing with what dartfmt does to
// artisanally-formatted Dart, while Stuart gets really frustrated when
// dealing with PRs from newer contributors who don't know how to make Dart
// readable. After much discussion, it was decided that files in the plugins
Copy link
Contributor

Choose a reason for hiding this comment

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

🤣 Well, I did ask.

(I was just thinking something like "Allow package maintainers to opt out of Dart autoformatting in order to use Flutter's manual formatting style instead.", but this certainly works.)

// and packages repos that really benefit from hand-formatting (e.g. files
// with large blobs of hex literals) could be opted-out of the requirement
// that they be autoformatted, so long as the code's owner was willing to
// bear the cost of this during code reviews.
// In the event that code ownership moves to someone who does not hold the
// same views as the original owner, the pragma can be removed and the file
// auto-formatted.
const String handFormattedExtension = '.dart';
const String handFormattedPragma = '// This file is hand-formatted.';

return files
.where((File file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I made this apply to all files rather than just Dart, mostly because it was easier to plug into the code where File objects were still around.

I would much rather this apply only to Dart, as this is strictly worse than clang-format off for clang-format-formatted files because the latter will prevent people with editors that are configured to auto-format from accidentally formatting the file when editing it. (I really, really wish dartfmt would support this for the same reason.)

What's the issue with just adding a file.basename.endsWith('.dart') check inside this lambda?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seemed weird to have this code know about file extensions too, but i'm happy to add it if you like

// See comment above near [handFormattedPragma].
return path.extension(file.path) != handFormattedExtension ||
!file.readAsLinesSync().contains(handFormattedPragma);
})
.map((File file) => path.relative(file.path, from: fromPath))
.where((String path) =>
// Ignore files in build/ directories (e.g., headers of frameworks)
Expand Down
2 changes: 1 addition & 1 deletion script/tool/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: flutter_plugin_tools
description: Productivity utils for flutter/plugins and flutter/packages
repository: https://github.com/flutter/plugins/tree/master/script/tool
version: 0.6.0+1
version: 0.7.0

dependencies:
args: ^2.1.0
Expand Down
37 changes: 37 additions & 0 deletions script/tool/test/format_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'package:args/command_runner.dart';
import 'package:file/file.dart';
import 'package:file/memory.dart';
import 'package:flutter_plugin_tools/src/common/core.dart';
import 'package:flutter_plugin_tools/src/common/file_utils.dart';
import 'package:flutter_plugin_tools/src/format_command.dart';
import 'package:path/path.dart' as p;
import 'package:test/test.dart';
Expand Down Expand Up @@ -106,6 +107,42 @@ void main() {
]));
});

test('does not format .dart files with pragma', () async {
const List<String> formattedFiles = <String>[
'lib/a.dart',
'lib/src/b.dart',
'lib/src/c.dart',
];
const String unformattedFile = 'lib/src/d.dart';
final Directory pluginDir = createFakePlugin(
'a_plugin',
packagesDir,
extraFiles: <String>[
...formattedFiles,
unformattedFile,
],
);

final p.Context posixContext = p.posix;
childFileWithSubcomponents(pluginDir, posixContext.split(unformattedFile))
.writeAsStringSync(
'// copyright bla bla\n// This file is hand-formatted.\ncode...');

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

expect(
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
ProcessCall(
getFlutterCommand(mockPlatform),
<String>[
'format',
..._getPackagesDirRelativePaths(pluginDir, formattedFiles)
],
packagesDir.path),
]));
});

test('fails if flutter format fails', () async {
const List<String> files = <String>[
'lib/a.dart',
Expand Down