[et] Adds a format command #50747

zanderso · 2024-02-18T00:10:38Z

loic-sharma · 2024-02-20T16:51:44Z

ci/bin/format.dart

        error('Found ${incorrect.length} python file${plural ? 's' : ''}'
            ' which ${plural ? 'were' : 'was'} formatted incorrectly:');
-        incorrect.forEach(stderr.writeln);
+        stdout.writeln('To fix, run:');


Should we recommend the --fix flag too? Perhaps:

Suggested change

stdout.writeln('To fix, run:');

stdout.writeln('To fix, run `dart ci/bin/format.dart --fix` or:');

Alternatively:

Suggested change

stdout.writeln('To fix, run:');

stdout.writeln('To fix, run `et format` or:');

Good idea. Done.

loic-sharma · 2024-02-20T16:53:37Z

tools/engine_tool/lib/src/commands/format_command.dart

+// run the program under ci/bin/format.dart. Instead, format.dart should be
+// moved under the engine_tool package and invoked by a function call. The file
+// ci/bin/format.dart should be split up so that each of its `FormatCheckers` is
+// in a seprate file under src/formatters, and they should be unit-tested.


Suggested change

// in a seprate file under src/formatters, and they should be unit-tested.

// in a separate file under src/formatters, and they should be unit-tested.

loic-sharma · 2024-02-20T16:54:23Z

tools/engine_tool/lib/src/commands/format_command.dart

+// run the program under ci/bin/format.dart. Instead, format.dart should be
+// moved under the engine_tool package and invoked by a function call. The file
+// ci/bin/format.dart should be split up so that each of its `FormatCheckers` is
+// in a seprate file under src/formatters, and they should be unit-tested.


Should we tag this with a TODO that links to flutter/flutter#132807?

Completely up to you, I don't mind either ways.

Good idea. Done.

loic-sharma · 2024-02-20T16:55:35Z

tools/engine_tool/lib/src/commands/format_command.dart

+
+/// The 'format' command.
+final class FormatCommand extends CommandBase {
+  /// Constructs the 'query' command.


This comment and the comment above on the class declaration aren't particularly helpful. I'd consider removing them. See: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-useless-documentation

Suggested change

/// Constructs the 'query' command.

/// Constructs the 'format' command.

loic-sharma · 2024-02-20T16:57:30Z

tools/engine_tool/lib/src/commands/flags.dart

+const String allFlag = 'all';
+const String builderFlag = 'builder';
+const String fixFlag = 'fix';
+const String printDiffsFlag = 'print-diffs';


Could you expand on the value of putting all flags in a single file? I imagine this will grow organically over time until it becomes unwieldy. It also hurts readability since in format_command.dart we can't see that printDiffsFlag is print-diffs.

The desire is to have flags named consistently across subcommands, while letting the subcommand flag help messages describe what they mean for the particular subcommand. Open to suggestions on a different way to achieve that.

loic-sharma · 2024-02-20T17:01:34Z

tools/engine_tool/lib/src/commands/format_command.dart

+  String get name => 'format';
+
+  @override
+  String get description => 'Fixes formatting.';


Consider:

Suggested change

String get description => 'Fixes formatting.';

String get description => 'Format source code files.';

Done with slightly different wording.

loic-sharma · 2024-02-20T17:04:11Z

tools/engine_tool/lib/src/commands/format_command.dart

+      ..addFlag(
+        fixFlag,
+        abbr: 'f',
+        help: 'Fixes formatting.',


Consider:

Suggested change

help: 'Fixes formatting.',

help: 'Updates source code files with formatted content.',

Done with slightly different wording.

loic-sharma · 2024-02-20T17:10:41Z

tools/engine_tool/lib/src/commands/format_command.dart

+  Future<int> run() async {
+    final bool all = argResults![allFlag]! as bool;
+    final bool fix = argResults![fixFlag]! as bool;
+    final bool printDiffs = argResults![printDiffsFlag]! as bool;


Currently if I want to see the diffs without updating files I need to do:

et --no-fix --print-diffs

Would it make sense to make print diffs default to the negation of fix? And make fix default to the negation of print diffs?

For example:

et format # Updates files, no diff et format --no-fix # Shows diff, does not update files et format --print-diffs # Shows diff, does not update files

Why not just one flag "--dry-run" and drop "--no-fix" and "--print-diffs" ?

Went with --dry-run.

…143816) flutter/engine@7002504...efc6994 2024-02-21 zanderso@users.noreply.github.com Make global builds uniquely named per platform (flutter/engine#50764) 2024-02-21 zanderso@users.noreply.github.com [et] Adds a format command (flutter/engine#50747) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This PR changes the format check on CI to use the command added in #50747. Additionally, while making this change, I noticed that the CI check was not checking the formatting of all files, and that as a result, files were present in the repo with incorrect formatting. I have fixed the formatting and fixed the check to always check all files.

zanderso requested review from johnmccutchan, liamappelbe and matanlurey February 20, 2024 16:01

loic-sharma reviewed Feb 20, 2024

View reviewed changes

loic-sharma approved these changes Feb 20, 2024

View reviewed changes

matanlurey approved these changes Feb 20, 2024

View reviewed changes

zanderso force-pushed the et-format branch 3 times, most recently from f5222b5 to 888dbd6 Compare February 20, 2024 21:05

zanderso removed the request for review from liamappelbe February 20, 2024 23:22

[et] Adds a format command

0f8cb09

zanderso force-pushed the et-format branch from 888dbd6 to 0f8cb09 Compare February 21, 2024 03:02

zanderso merged commit 3116c41 into flutter:main Feb 21, 2024

zanderso deleted the et-format branch February 21, 2024 03:40

zanderso mentioned this pull request Feb 21, 2024

Use 'et format' in CI. Check formatting of all files in CI #50810

Merged

engine-flutter-autoroll mentioned this pull request Feb 21, 2024

Roll Flutter Engine from 700250436e3f to efc69946cb1e (2 revisions) flutter/flutter#143816

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 21, 2024

3116c41e3 [et] Adds a format command (flutter/engine#50747)

18de51a

	stdout.writeln('To fix, run:');
	stdout.writeln('To fix, run `dart ci/bin/format.dart --fix` or:');

	stdout.writeln('To fix, run:');
	stdout.writeln('To fix, run `et format` or:');

	// in a seprate file under src/formatters, and they should be unit-tested.
	// in a separate file under src/formatters, and they should be unit-tested.

	/// Constructs the 'query' command.
	/// Constructs the 'format' command.

	String get description => 'Fixes formatting.';
	String get description => 'Format source code files.';

	help: 'Fixes formatting.',
	help: 'Updates source code files with formatted content.',

[et] Adds a format command #50747

[et] Adds a format command #50747

Uh oh!

Conversation

zanderso commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zanderso commented Feb 18, 2024 •

edited

Loading

loic-sharma Feb 20, 2024 •

edited

Loading