[gen_l10n] Add option to format generated localizations files #109171

thkim1011 · 2022-08-08T18:18:33Z

See title. This adds a --format flag to the flutter gen-l10n command. We do this by returning the list of generated files from LocalizationsGenerator.writeOutputFiles and doing post processing from the generate_localizations.dart file. This should also be the way of adding any additional post processing steps to the gen-l10n command going forward.

Also, something else to note is that all the tests in generate_localizations_test.dart has been calling the command twice. I didn't think that the check for exitStatus was necessary, so I removed them.

Fixes #98122

Relevant PRs: #108563, #98701.

flutter-dashboard · 2022-08-08T18:18:36Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

thkim1011 · 2022-08-08T18:19:46Z

@christopherfujino @HansMuller can I get a quick look at the changes just to make sure this makes sense before I go and update generate_localizations_test.dart?

HansMuller

LGTM

HansMuller · 2022-08-08T18:29:24Z

packages/flutter_tools/lib/src/commands/generate_localizations.dart

    );
+    argParser.addFlag(
+      'format',
+      help: 'When specified, the `dart format` command is run after generating the files.'


"the files" is OK, since the reader is unlikely to think that we're referring to .arb files (or whatever), but it might be a clearer to clarify this a little.

cedvdb · 2022-08-09T07:46:08Z

I'm confused why this is opt-in and even optional. In what situation wouldn't you need the file to be formatted like the rest of the project ? How will this play out with vs code auto gen of localization on save ?

christopherfujino · 2022-08-09T18:09:33Z

I'm confused why this is opt-in and even optional. In what situation wouldn't you need the file to be formatted like the rest of the project ? How will this play out with vs code auto gen of localization on save ?

if you don't use dart format on your codebase
if we don't make this opt-in, it will be a breaking change to customers who check in their generated files.

christopherfujino · 2022-08-09T18:12:48Z

packages/flutter_tools/lib/src/commands/generate_localizations.dart


  final FileSystem _fileSystem;
  final Logger _logger;
+  final Artifacts? _artifacts;


does this need to be nullable?

Yes, since globals.artifacts is nullable.

christopherfujino · 2022-08-09T18:16:05Z

packages/flutter_tools/lib/src/commands/generate_localizations.dart

+      }
+      final String dartBinary = _artifacts!.getHostArtifact(HostArtifact.engineDartBinary).path;
+      final List<String> command = <String>[dartBinary, 'format', ...outputFileList];
+      final Process process = await _processManager.start(command);


If you don't need to interact with the sub-process, calling .run() is cleaner:

final ProcessResult result = await _processManager.run(command); if (result.exitCode != 0) {

christopherfujino · 2022-08-09T18:17:50Z

packages/flutter_tools/lib/executable.dart

    logger: globals.logger,
+    artifacts: globals.artifacts,
+    processManager: globals.processManager,
+


Suggested change

christopherfujino

This approach looks good to me

cedvdb · 2022-08-09T18:55:59Z

if you don't use dart format on your codebase

if we don't make this opt-in, it will be a breaking change to customers who check in their generated files.

How can formatting be a breaking change ? It stills obeys whatever analys options.yaml that's there right ? so any CI dart analyse will still pass

how will auto generation be handled here ? (ctrl + s instead of running the command)

thkim1011 · 2022-08-09T19:26:49Z

Not necessarily breaking, but it would be a bit annoying/confusing having to commit unexpected changes due to formatting changes.

For automatic generation, I'll add a format option to the yaml config.

AlexV525 · 2022-08-10T04:29:37Z

I think #108563 has already done what this PR has done. And we have another test case issue with test timeout that need to be resolved. Also, @thkim1011 is on the review list.

AlexV525 · 2022-08-10T07:06:50Z

packages/flutter_tools/lib/src/commands/generate_localizations.dart

-    } on L10nException catch (e) {
-      throwToolExit(e.message);
+    // All other post processing.
+    if (boolArg('format') ?? false) {


This will only apply to the format when using flutter tools directly. Generalizing is also included in building targets.

AlexV525 · 2022-08-10T07:09:35Z

if you don't use dart format on your codebase

If you don't, #103414 should also be considered as a breaking change.

if we don't make this opt-in, it will be a breaking change to customers who check in their generated files.

Generated files can be affected only once after they update their SDK. We do have a couple of patches during SDK updates like ignoring frame rates limitation when building iOS apps.

cedvdb · 2022-08-10T19:09:16Z

Not necessarily breaking, but it would be a bit annoying/confusing having to commit unexpected changes due to formatting changes.

That's only valid for the first localization gen and only because it did not originally obey formatting rules.

thkim1011 · 2022-08-11T19:49:00Z

I think #108563 has already done what this PR has done. And we have another test case issue with test timeout that need to be resolved. Also, @thkim1011 is on the review list.

@AlexV525 is there a specific reason why we should pass in artifacts and processManager into LocalizationsGenerator and doing the formatting within the LocalizationsGenerator class in your PR? I think this is the only difference between our PRs? I feel that any sort of post processing on the generated files don't really belong in the LocalizationsGenerator class, but I'm not completely opposed to your PR either.

christopherfujino · 2022-08-11T20:54:25Z

packages/flutter_tools/test/commands.shard/hermetic/generate_localizations_test.dart

    fileSystem = MemoryFileSystem.test();
+    logger = BufferLogger.test();
+    artifacts = Artifacts.test();
+    processManager = const LocalProcessManager();


Is this running real sub-processes? If so, it will need a real file system, and should be moved to the commands.shard/permable directory.

I changed this so that we use a FakeProcessManager instead.

AlexV525 · 2022-08-11T21:54:41Z

I feel that any sort of post processing on the generated files don't really belong in the LocalizationsGenerator class

IMO it belongs to the generator, something like who generates them should format them. In #108563 the generator obtain the artifacts and the process manager not only from the context runner, but also from a build target's environment.

jonahwilliams · 2022-08-17T22:48:12Z

packages/flutter_tools/lib/src/commands/generate_localizations.dart

-      throwToolExit(e.message);
+    // All other post processing.
+    if (format) {
+      if (_artifacts == null) {


Seems like this would be a programming error, no? When is artifacts null?

Gotcha, I just checked the rest of the commands in executable.dart and they seem to just pass in artifacts as artifacts! so I'll do that.

jonahwilliams

LGTM with nit

AlexV525 · 2022-08-19T03:48:58Z

I don't see any tests that verified if generated files are well-formatted.

jonahwilliams · 2022-08-19T04:06:30Z

Whether or not a file is formatted is something that dartfmt knows, but its not the tools responsibility to test. If we've tested that dartfmt has been invoked that is good enough for me

…flutter/flutter#109171)

…#167029) Migration of Localization Generation [Option 1](https://docs.flutter.dev/release/breaking-changes/flutter-generate-i10n-source#migration-guide) breaks `dart format . --set-exit-if-changed` checks (as commonly used in pipelines), as the generated file does not comply the `dart format` rules. This PR: - Formats the generated localization files by default - In contrast to [this comment](#109171 (comment)) I don't see it as breaking as the files are generated dynamically on every change and there's no need in migrating them. Also any tests should not fail as either the format was done manually afterwards anyways, or the formatting was not checked at all (in third party apps / libraries) as the output had not a correct format in any use case of checking via `dart format`. - Add tests for `no-format` - Add option to add arbitrary arguments to the `dart format` command to allow customized formatting Closes #167982 Contributes to: #102983 (comment) *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

…102983) (flutter#167029) Migration of Localization Generation [Option 1](https://docs.flutter.dev/release/breaking-changes/flutter-generate-i10n-source#migration-guide) breaks `dart format . --set-exit-if-changed` checks (as commonly used in pipelines), as the generated file does not comply the `dart format` rules. This PR: - Formats the generated localization files by default - In contrast to [this comment](flutter#109171 (comment)) I don't see it as breaking as the files are generated dynamically on every change and there's no need in migrating them. Also any tests should not fail as either the format was done manually afterwards anyways, or the formatting was not checked at all (in third party apps / libraries) as the output had not a correct format in any use case of checking via `dart format`. - Add tests for `no-format` - Add option to add arbitrary arguments to the `dart format` command to allow customized formatting Closes flutter#167982 Contributes to: flutter#102983 (comment) *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

thkim1011 added 4 commits August 8, 2022 10:19

init

ba240d2

fix

560c5d2

fix 2

4390f41

fix 3

e2b4505

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 8, 2022

thkim1011 requested review from HansMuller and christopherfujino August 8, 2022 18:18

HansMuller approved these changes Aug 8, 2022

View reviewed changes

christopherfujino reviewed Aug 9, 2022

View reviewed changes

AlexV525 reviewed Aug 10, 2022

View reviewed changes

tests

e8b121f

This was referenced Aug 11, 2022

fix(flutter_tools): Format gen-l10n output files with dart format #98701

Closed

[gen_l10n] Format generated localizations #108563

Closed

christopherfujino reviewed Aug 11, 2022

View reviewed changes

thkim1011 added 2 commits August 16, 2022 13:17

fix tests

7433720

clarify help text

3587233

thkim1011 added 5 commits August 17, 2022 13:02

fix all tests

f0edd09

fix formatting?

e6efce4

add second test

1e784ac

unused import

dfde420

remove print

f78dba7

thkim1011 requested a review from christopherfujino August 17, 2022 20:40

trailing spaces

5ff4681

jonahwilliams reviewed Aug 17, 2022

View reviewed changes

jonahwilliams approved these changes Aug 17, 2022

View reviewed changes

thkim1011 added 3 commits August 18, 2022 12:44

Merge remote-tracking branch 'upstream/master' into tae/add-format

0d26b65

artifacts is never null

a76c793

fix

c31e810

thkim1011 merged commit 646666f into flutter:master Aug 19, 2022

thkim1011 deleted the tae/add-format branch August 19, 2022 21:03

This was referenced Aug 20, 2022

Roll Flutter from 663bb66bfd90 to 723609bff1f7 (29 revisions) flutter/packages#2477

Merged

Roll Flutter from 663bb66bfd90 to 723609bff1f7 (29 revisions) flutter/plugins#6297

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 20, 2022

646666f [gen_l10n] Add option to format generated localizations files (…

2aec767

…flutter/flutter#109171)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 20, 2022

646666f [gen_l10n] Add option to format generated localizations files (…

7852d79

…flutter/flutter#109171)

Gustl22 mentioned this pull request Apr 12, 2025

feat: Arbitrary format options for localizations generation (#102983) #167029

Merged

9 tasks

[gen_l10n] Add option to format generated localizations files #109171

[gen_l10n] Add option to format generated localizations files #109171

Uh oh!

Conversation

thkim1011 commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Aug 8, 2022

Uh oh!

thkim1011 commented Aug 8, 2022

Uh oh!

HansMuller left a comment

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!

cedvdb commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christopherfujino commented Aug 9, 2022

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!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

cedvdb commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thkim1011 commented Aug 9, 2022

Uh oh!

AlexV525 commented Aug 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexV525 commented Aug 10, 2022

Uh oh!

cedvdb commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thkim1011 commented Aug 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexV525 commented Aug 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

AlexV525 commented Aug 19, 2022

Uh oh!

jonahwilliams commented Aug 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

thkim1011 commented Aug 8, 2022 •

edited

Loading

cedvdb commented Aug 9, 2022 •

edited

Loading

cedvdb commented Aug 9, 2022 •

edited

Loading

cedvdb commented Aug 10, 2022 •

edited

Loading