Skip to content

[gen_l10n] Format generated localizations#108563

Closed
AlexV525 wants to merge 22 commits intoflutter:masterfrom
AlexV525:format-generated-localizations
Closed

[gen_l10n] Format generated localizations#108563
AlexV525 wants to merge 22 commits intoflutter:masterfrom
AlexV525:format-generated-localizations

Conversation

@AlexV525
Copy link
Member

@AlexV525 AlexV525 commented Jul 28, 2022 •
edited
Loading

Generated localizations dart files are always not well-formatted. Previously I've applied #103414 but it's not enough for analysis apparently. This PR adds the ability to format generated files when running flutter gen-l10n.

  • generateLocalizations is now Future<LocalizationsGenerator.
  • loadResources and writeOutputFiles are combined to generate.
  • writeOutputFiles is no longer public visible.
  • _formatOutputFiles will format only output files.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 28, 2022
@AlexV525 AlexV525 added the a: internationalization Supporting other languages or locales. (aka i18n) label Jul 28, 2022
AlexV525
AlexV525 commented Jul 29, 2022
View reviewed changes
packages/flutter_tools/lib/src/localizations/gen_l10n_templates.dart
'an issue with the localizations generation tool. Please file an issue '
'on GitHub with a reproducible sample app and the gen-l10n configuration '
'that was used.'
'that was used.',
Copy link
Member Author

@AlexV525 AlexV525 Jul 29, 2022

Choose a reason for hiding this comment

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

These fix require_trailing_commas.

@AlexV525 AlexV525 force-pushed the format-generated-localizations branch from 57976b1 to f58f52f Compare July 29, 2022 10:32
@AlexV525

This comment was marked as resolved.

Sign in to view
@AlexV525 AlexV525 changed the title [gen-l10n] Format generated localizations [gen_l10n] Format generated localizations Jul 30, 2022
@AlexV525 AlexV525 marked this pull request as ready for review July 30, 2022 04:20
@AlexV525
Copy link
Member Author

AlexV525 commented Jul 30, 2022

Seeing a couple of timeout exceptions when running tests, but all passed after re-run.

jonahwilliams
jonahwilliams reviewed Jul 30, 2022
View reviewed changes
@AlexV525 AlexV525 marked this pull request as draft July 31, 2022 16:37
@christopherfujino
Copy link
Contributor

christopherfujino commented Aug 1, 2022

Adding @thkim1011 to review.

@AlexV525
Copy link
Member Author

AlexV525 commented Aug 5, 2022

Global artifacts and the process manager should be passed at least from the command and the generateLocalizations at present, same as other commands. The FormatCommand uses the same mechanism to format files.

@AlexV525 AlexV525 marked this pull request as ready for review August 5, 2022 08:44
@AlexV525 AlexV525 requested a review from jonahwilliams August 5, 2022 08:44
@AlexV525
Copy link
Member Author

AlexV525 commented Aug 5, 2022

And the format process may not fit the default timeout (2 seconds) with tests...

@thkim1011 thkim1011 mentioned this pull request Aug 8, 2022
Merged
@christopherfujino
Copy link
Contributor

christopherfujino commented Aug 11, 2022

@thkim1011 should we close this as superceded by #109171 ?

@thkim1011
Copy link
Contributor

thkim1011 commented Aug 19, 2022

Hey @AlexV525, although we really appreciate the contribution, I merged in my changes (#109171) as I would prefer to keep the dart formatting logic separate from the main gen_l10n logic for now, and the consensus from the team was that we should keep --format as an option (you can also add format: true to your l10n.yaml).

@thkim1011 thkim1011 closed this Aug 19, 2022
@AlexV525 AlexV525 deleted the format-generated-localizations branch December 10, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thkim1011 thkim1011 Awaiting requested review from thkim1011

@jonahwilliams jonahwilliams Awaiting requested review from jonahwilliams

1 more reviewer

@christopherfujino christopherfujino christopherfujino left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: internationalization Supporting other languages or locales. (aka i18n) tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AlexV525 @christopherfujino @thkim1011 @jonahwilliams