Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gen_l10n] Synthetic package generation by default #62395

Merged
merged 83 commits into from Aug 31, 2020

Conversation

shihaohong
Copy link
Contributor

@shihaohong shihaohong commented Jul 28, 2020

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

  1. 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
  1. 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';
  1. 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.

@fluttergithubbot fluttergithubbot added team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Jul 28, 2020
jonahwilliams
jonahwilliams previously approved these changes Jul 28, 2020
Copy link
Member

@jonahwilliams jonahwilliams left a comment

LGTM with nits

@@ -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');
Copy link
Member

@jonahwilliams jonahwilliams Jul 28, 2020

Choose a reason for hiding this comment

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

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

}) {
if (useSyntheticPackage) {
outputDirectory = _fs.directory(defaultSyntheticPackagePath);
return;
Copy link
Member

@jonahwilliams jonahwilliams Jul 28, 2020

Choose a reason for hiding this comment

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

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

@@ -36,6 +36,7 @@ Future<void> generateLocalizations({
'bin',
'gen_l10n.dart',
);

final ProcessResult result = await processManager.run(<String>[
Copy link
Member

@jonahwilliams jonahwilliams Jul 28, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@shihaohong shihaohong Jul 29, 2020

Choose a reason for hiding this comment

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

I just added the feature and a test. PTAL

// 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()) {
Copy link
Member

@jonahwilliams jonahwilliams Aug 26, 2020

Choose a reason for hiding this comment

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

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

);

try {
await generateLocalizations(
Copy link
Member

@jonahwilliams jonahwilliams Aug 26, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@jonahwilliams jonahwilliams Aug 26, 2020

Choose a reason for hiding this comment

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

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

});
}

class MockProcessManager implements ProcessManager {
Copy link
Member

@jonahwilliams jonahwilliams Aug 26, 2020

Choose a reason for hiding this comment

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

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


expect(
() async {
return await generateLocalizationsSyntheticPackage(
Copy link
Member

@jonahwilliams jonahwilliams Aug 26, 2020

Choose a reason for hiding this comment

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

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

@jonahwilliams jonahwilliams self-requested a review Aug 26, 2020
@shihaohong shihaohong dismissed jonahwilliams’s stale review Aug 27, 2020

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

Copy link
Member

@jonahwilliams jonahwilliams left a comment

LGTM

ship it!

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Aug 27, 2020

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.

@shihaohong
Copy link
Contributor Author

shihaohong commented Aug 29, 2020

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

@shihaohong shihaohong merged commit fd22fc3 into flutter:master Aug 31, 2020
3 of 4 checks passed
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
* 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 shihaohong deleted the gen_synthetic_l10n branch Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: internationalization Supporting other languages or locales. (aka i18n) severe: API break Backwards-incompatible API changes. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants