[gen_l10n] Synthetic package generation by default #62395

shihaohong · 2020-07-28T09:22:53Z

Description

Modifies the gen_l10n tool to use synthetic packages by default. This allows developers to generate the localization output files in a package that will not need to be checked into the user's Flutter project.

While this is a breaking change to the existing users of the tool, I believe it is justified since the tool is still in its early stages and not publicized. I would prefer that the default behavior of the tool be the preferred method of using synthetic packages rather than not. However, I'd like to get some feedback on what people think about this breaking change.

Migration Guide

To not use synthetic packages

If calling gen_l10n via the command line, simply add --no-synthetic-package to your call:

${FLUTTER}/bin/dart ${FLUTTER}/dev/tools/ --no-synthetic-package {other options}

If using l10n.yaml, add the following option:

# other options
synthetic-package: false

To use synthetic packages

In your Flutter project, add the following to your pubspec.yaml file:

flutter:
  ## note, this is NOT dependencies: flutter: generate, but is instead
  ## flutter: generate, which is usually further down in your pubspec.yaml
  ## file.
  generate: true

After generating the localizations files for your Flutter application (you can do this through calling flutter run), simply update the location where you are importing your localization file. For example,
in the stocks app:

// previous import:
// import 'i18n/stock_strings.dart'; 

// now, import the synthetic package instead
import 'package:flutter_gen/gen_l10n/stock_strings.dart';

Remove the generated localizations output files from where they previously were.

Tests

I added the following tests:

Modified existing tests to understand expect generated files to be in the synthetic package directory by default
Modified existing tests to generate output files in custom directories if the synthetic package flag is turned off.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

jonahwilliams

LGTM with nits

jonahwilliams · 2020-07-28T15:57:42Z

dev/tools/localization/gen_l10n.dart

@@ -13,6 +13,8 @@ import 'gen_l10n_templates.dart';
 import 'gen_l10n_types.dart';
 import 'localizations_utils.dart';

+String defaultSyntheticPackagePath = path.join('.dart_tool', 'flutter_gen', 'gen_l10n');


Could you leave a comment linking this path with the useSyntheticPackage setting in the flutter tool?

jonahwilliams · 2020-07-28T15:58:06Z

dev/tools/localization/gen_l10n.dart

+  }) {
+    if (useSyntheticPackage) {
+      outputDirectory = _fs.directory(defaultSyntheticPackagePath);
+      return;


nit: if you have a return here you don't need the else below.

jonahwilliams · 2020-07-28T15:59:33Z

packages/flutter_tools/lib/src/build_system/targets/localizations.dart

@@ -36,6 +36,7 @@ Future<void> generateLocalizations({
    'bin',
    'gen_l10n.dart',
  );
+
  final ProcessResult result = await processManager.run(<String>[


There is one small missing feature: you could check that generate is true and if not warn that the sources won't be importable.

I just added the feature and a test. PTAL

jonahwilliams · 2020-08-26T20:44:59Z

packages/flutter_tools/lib/src/dart/generate_synthetic_packages.dart

+  // If pubspec.yaml has generate:true and if l10n.yaml exists in the
+  // root project directory, check to see if a synthetic package should
+  // be generated for gen_l10n.
+  if (l10nYamlFile.existsSync()) {


you could drastically reduce the nesting here by re-arranging the code a bit:

if (!exists) { return; } if (condition that causes type error) { ... throwToolExit(..); }

i.e. check for errors first, and early return/throw

jonahwilliams · 2020-08-26T20:49:03Z

packages/flutter_tools/test/general.shard/build_system/targets/localizations_test.dart

+    );
+
+    try {
+      await generateLocalizations(


expect(() => ..., throwsA(isA<Exception>())

you should also check that the logger error text contains some part of the message you added

jonahwilliams · 2020-08-26T20:50:36Z

packages/flutter_tools/test/general.shard/dart/generate_synthetic_packages_test.dart

+  });
+}
+
+class MockProcessManager implements ProcessManager {


Please don't implement your own mocks. Either use the Mock/Fake class from mockito, or use the FakeProcessManager

jonahwilliams · 2020-08-26T20:51:17Z

packages/flutter_tools/test/general.shard/dart/generate_synthetic_packages_test.dart

+
+    expect(
+      () async {
+        return await generateLocalizationsSyntheticPackage(


return await does not really do anything, you can return the future directly

Already requested a re-review, but preexisting approval still remains

jonahwilliams

LGTM

ship it!

fluttergithubbot · 2020-08-27T17:53:02Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite testonly_devicelab_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

…c_l10n

shihaohong · 2020-08-29T01:30:52Z

Google testing has actually passed. The GitHub check is just stuck at pending. I'll merge this once flutter-build goes green.

* synthetic packages by default in gen_l10n tool * Refactor default path for synthetic package * Remove unused import * Code cleanup * Further improvements to help text * Refactor synthetic package path * Remove newlines * Test cleanup * clean up logic in inputs and outputs list function * Update l10n.yaml usage * only add option if value is non-null * Update stocks app as proof of concept for synthetic package usage * Address nits * print pubspec contents * add print statements * Do not allow null value for useSyntheticPackage * + * + * + * + * Cleanup * Add test * Fix text * Dont parse pubspec directly * Test using context * WIP: generate synthetic packages on pub get -- needs tests * Allow null value * Update null handling * Refactor to properly handle null case * Fix yamlMap condition * Fix yaml node for real * WIP: struggling to write tests * WIP - take absolute path as an option * Add tests * Use environment project directory for synthetic package generation pathway * Fix typo * Improve help text * Update defaults * Remove unauthorized path import * Fix pathing issues at synthetic package generation * Fix typo in test * Use path.join so projectDir matches up based on OS * Fix Windows pathing in test * Remove unnecessary replaceApp code for projectDir.path * Use globals.fs.currentDirectory.path in resident_runner_test.dart * Fix merge conflict * Add test to ensure that synthetic package is generated on pub get * Fix resident_runner_test.dart tests * Fix tests * Use package:file instead of dart:io * WIP - exploration * Remove synthetic package use from stocks example * Update integration test to not use synthetic packages * Remove trailing whitespace * flutter pub get runs synth package generation * Remove more print statements * Add license header * WIP - minimally working pub.get * Use own MockBuildSystem * Modify test and implementation to be a little cleaner * Fix flutter pub get invocation * Use synthetic packages in stocks app * Revert "Use synthetic packages in stocks app" This reverts commit 45bf249. * Add environment and buildSystem params to flutter test * Address code review feedback * + * Isolate codegen into its own API * Fix imports * Slight refactor * Add one more test for no l10n.yaml file * Remove unneeded mock class and import in pub_get_test.dart * More code review feedback * Remove unnecessary imports * Remove `return await`s that I missed * use arrow functions instead

shihaohong added 12 commits July 28, 2020 16:03

synthetic packages by default in gen_l10n tool

8961ab0

Refactor default path for synthetic package

c9d2ac4

Remove unused import

561aade

Code cleanup

0144838

Further improvements to help text

960e222

Refactor synthetic package path

76d5958

Remove newlines

b7bbf09

Test cleanup

d799c1c

clean up logic in inputs and outputs list function

080e3b2

Update l10n.yaml usage

e08b4c9

only add option if value is non-null

cab66ee

Update stocks app as proof of concept for synthetic package usage

d09913e

fluttergithubbot added c: contributor-productivity tool labels Jul 28, 2020

googlebot added the cla: yes label Jul 28, 2020

shihaohong requested review from jonahwilliams and HansMuller July 28, 2020 09:23

jonahwilliams previously approved these changes Jul 28, 2020

View reviewed changes

jonahwilliams reviewed Jul 28, 2020

View reviewed changes

shihaohong added 11 commits July 29, 2020 16:23

Address nits

a83a7bc

print pubspec contents

369572b

add print statements

c54292b

Do not allow null value for useSyntheticPackage

cfce1a1

+

9bfccce

+

903a270

+

1c74b63

+

89556a0

Cleanup

7ed56d7

Add test

d3751f9

Fix text

17cb9be

shihaohong added 6 commits August 26, 2020 20:55

+

8529dd0

Isolate codegen into its own API

d20a0a8

Fix imports

6c95419

Slight refactor

1546ddf

Add one more test for no l10n.yaml file

1a8e6fc

Remove unneeded mock class and import in pub_get_test.dart

e7385f6

jonahwilliams reviewed Aug 26, 2020

View reviewed changes

jonahwilliams self-requested a review August 26, 2020 20:51

More code review feedback

583275d

shihaohong added 3 commits August 27, 2020 11:57

Remove unnecessary imports

adef381

Remove return awaits that I missed

ecde0fb

use arrow functions instead

b612baf

jonahwilliams approved these changes Aug 27, 2020

View reviewed changes

shihaohong added the waiting for tree to go green label Aug 27, 2020

fluttergithubbot removed the waiting for tree to go green label Aug 27, 2020

Merge branch 'master' of github.com:flutter/flutter into gen_syntheti…

c5bcc7b

…c_l10n

shihaohong added the waiting for tree to go green label Aug 27, 2020

shihaohong merged commit fd22fc3 into flutter:master Aug 31, 2020

This was referenced Sep 1, 2020

Use Synthetic Packages for localizations flutter/gallery#282

Merged

Move gen_l10n into flutter_tools #65025

Merged

shihaohong deleted the gen_synthetic_l10n branch December 3, 2020 11:33

pd4d10 mentioned this pull request Feb 3, 2021

Use Synthetic Packages for localizations pd4d10/git-touch#185

Closed

SandroMaglione mentioned this pull request Apr 5, 2021

Cannot resolve flutter i18n classes inside gen_I10n using build_runner #79776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gen_l10n] Synthetic package generation by default #62395

[gen_l10n] Synthetic package generation by default #62395

shihaohong commented Jul 28, 2020 •

edited

Loading

jonahwilliams left a comment

jonahwilliams Jul 28, 2020

jonahwilliams Jul 28, 2020

jonahwilliams Jul 28, 2020

shihaohong Jul 29, 2020

jonahwilliams Aug 26, 2020

jonahwilliams Aug 26, 2020

jonahwilliams Aug 26, 2020

jonahwilliams Aug 26, 2020

jonahwilliams Aug 26, 2020

jonahwilliams left a comment

fluttergithubbot commented Aug 27, 2020

shihaohong commented Aug 29, 2020

[gen_l10n] Synthetic package generation by default #62395

[gen_l10n] Synthetic package generation by default #62395

Conversation

shihaohong commented Jul 28, 2020 • edited Loading

Description

Migration Guide

To not use synthetic packages

To use synthetic packages

Tests

Checklist

Breaking Change

jonahwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

fluttergithubbot commented Aug 27, 2020

shihaohong commented Aug 29, 2020

shihaohong commented Jul 28, 2020 •

edited

Loading