From 646666f8e794cbae0400930c382f637601da0707 Mon Sep 17 00:00:00 2001 From: Tae Hyung Kim Date: Fri, 19 Aug 2022 14:03:42 -0700 Subject: [PATCH] [gen_l10n] Add option to format generated localizations files (#109171) * init * fix * fix 2 * fix 3 * tests * fix tests * clarify help text * fix all tests * fix formatting? * add second test * unused import * remove print * trailing spaces * artifacts is never null * fix --- packages/flutter_tools/lib/executable.dart | 2 + .../src/commands/generate_localizations.dart | 124 +++++++++++------- .../lib/src/localizations/gen_l10n.dart | 23 ++-- .../localizations/localizations_utils.dart | 7 + .../hermetic/generate_localizations_test.dart | 114 ++++++++++++++-- 5 files changed, 200 insertions(+), 70 deletions(-) diff --git a/packages/flutter_tools/lib/executable.dart b/packages/flutter_tools/lib/executable.dart index 944ec4ae07..94b4f51bc6 100644 --- a/packages/flutter_tools/lib/executable.dart +++ b/packages/flutter_tools/lib/executable.dart @@ -178,6 +178,8 @@ List generateCommands({ GenerateLocalizationsCommand( fileSystem: globals.fs, logger: globals.logger, + artifacts: globals.artifacts!, + processManager: globals.processManager, ), InstallCommand(), LogsCommand(), diff --git a/packages/flutter_tools/lib/src/commands/generate_localizations.dart b/packages/flutter_tools/lib/src/commands/generate_localizations.dart index 74a6678a01..f7e320b857 100644 --- a/packages/flutter_tools/lib/src/commands/generate_localizations.dart +++ b/packages/flutter_tools/lib/src/commands/generate_localizations.dart @@ -2,8 +2,12 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'package:process/process.dart'; + +import '../artifacts.dart'; import '../base/common.dart'; import '../base/file_system.dart'; +import '../base/io.dart'; import '../base/logger.dart'; import '../globals.dart' as globals; import '../localizations/gen_l10n.dart'; @@ -21,9 +25,13 @@ class GenerateLocalizationsCommand extends FlutterCommand { GenerateLocalizationsCommand({ required FileSystem fileSystem, required Logger logger, + required Artifacts artifacts, + required ProcessManager processManager, }) : _fileSystem = fileSystem, - _logger = logger { + _logger = logger, + _artifacts = artifacts, + _processManager = processManager { argParser.addOption( 'arb-dir', defaultsTo: globals.fs.path.join('lib', 'l10n'), @@ -180,10 +188,16 @@ class GenerateLocalizationsCommand extends FlutterCommand { 'Localizations.of(context), removing the need for null checking in ' 'user code.' ); + argParser.addFlag( + 'format', + help: 'When specified, the "dart format" command is run after generating the localization files.' + ); } final FileSystem _fileSystem; final Logger _logger; + final Artifacts _artifacts; + final ProcessManager _processManager; @override String get description => 'Generate localizations for the current project.'; @@ -196,6 +210,10 @@ class GenerateLocalizationsCommand extends FlutterCommand { @override Future runCommand() async { + List outputFileList; + + bool format = boolArg('format') ?? false; + if (_fileSystem.file('l10n.yaml').existsSync()) { final LocalizationOptions options = parseLocalizationsOptions( file: _fileSystem.file('l10n.yaml'), @@ -207,57 +225,71 @@ class GenerateLocalizationsCommand extends FlutterCommand { 'To use the command line arguments, delete the l10n.yaml file in the ' 'Flutter project.\n\n' ); - generateLocalizations( + outputFileList = generateLocalizations( logger: _logger, options: options, projectDir: _fileSystem.currentDirectory, fileSystem: _fileSystem, - ); - return FlutterCommandResult.success(); + ).outputFileList; + format = format || options.format; + } else { + final String inputPathString = stringArgDeprecated('arb-dir')!; // Has default value, cannot be null. + final String? outputPathString = stringArgDeprecated('output-dir'); + final String outputFileString = stringArgDeprecated('output-localization-file')!; // Has default value, cannot be null. + final String templateArbFileName = stringArgDeprecated('template-arb-file')!; // Has default value, cannot be null. + final String? untranslatedMessagesFile = stringArgDeprecated('untranslated-messages-file'); + final String classNameString = stringArgDeprecated('output-class')!; // Has default value, cannot be null. + final List preferredSupportedLocales = stringsArg('preferred-supported-locales'); + final String? headerString = stringArgDeprecated('header'); + final String? headerFile = stringArgDeprecated('header-file'); + final bool useDeferredLoading = boolArgDeprecated('use-deferred-loading'); + final String? inputsAndOutputsListPath = stringArgDeprecated('gen-inputs-and-outputs-list'); + final bool useSyntheticPackage = boolArgDeprecated('synthetic-package'); + final String? projectPathString = stringArgDeprecated('project-dir'); + final bool areResourceAttributesRequired = boolArgDeprecated('required-resource-attributes'); + final bool usesNullableGetter = boolArgDeprecated('nullable-getter'); + + precacheLanguageAndRegionTags(); + + try { + outputFileList = (LocalizationsGenerator( + fileSystem: _fileSystem, + inputPathString: inputPathString, + outputPathString: outputPathString, + templateArbFileName: templateArbFileName, + outputFileString: outputFileString, + classNameString: classNameString, + preferredSupportedLocales: preferredSupportedLocales, + headerString: headerString, + headerFile: headerFile, + useDeferredLoading: useDeferredLoading, + inputsAndOutputsListPath: inputsAndOutputsListPath, + useSyntheticPackage: useSyntheticPackage, + projectPathString: projectPathString, + areResourceAttributesRequired: areResourceAttributesRequired, + untranslatedMessagesFile: untranslatedMessagesFile, + usesNullableGetter: usesNullableGetter, + logger: _logger, + ) + ..loadResources() + ..writeOutputFiles()) + .outputFileList; + } on L10nException catch (e) { + throwToolExit(e.message); + } } - final String inputPathString = stringArgDeprecated('arb-dir')!; // Has default value, cannot be null. - final String? outputPathString = stringArgDeprecated('output-dir'); - final String outputFileString = stringArgDeprecated('output-localization-file')!; // Has default value, cannot be null. - final String templateArbFileName = stringArgDeprecated('template-arb-file')!; // Has default value, cannot be null. - final String? untranslatedMessagesFile = stringArgDeprecated('untranslated-messages-file'); - final String classNameString = stringArgDeprecated('output-class')!; // Has default value, cannot be null. - final List preferredSupportedLocales = stringsArg('preferred-supported-locales'); - final String? headerString = stringArgDeprecated('header'); - final String? headerFile = stringArgDeprecated('header-file'); - final bool useDeferredLoading = boolArgDeprecated('use-deferred-loading'); - final String? inputsAndOutputsListPath = stringArgDeprecated('gen-inputs-and-outputs-list'); - final bool useSyntheticPackage = boolArgDeprecated('synthetic-package'); - final String? projectPathString = stringArgDeprecated('project-dir'); - final bool areResourceAttributesRequired = boolArgDeprecated('required-resource-attributes'); - final bool usesNullableGetter = boolArgDeprecated('nullable-getter'); - - precacheLanguageAndRegionTags(); - - try { - LocalizationsGenerator( - fileSystem: _fileSystem, - inputPathString: inputPathString, - outputPathString: outputPathString, - templateArbFileName: templateArbFileName, - outputFileString: outputFileString, - classNameString: classNameString, - preferredSupportedLocales: preferredSupportedLocales, - headerString: headerString, - headerFile: headerFile, - useDeferredLoading: useDeferredLoading, - inputsAndOutputsListPath: inputsAndOutputsListPath, - useSyntheticPackage: useSyntheticPackage, - projectPathString: projectPathString, - areResourceAttributesRequired: areResourceAttributesRequired, - untranslatedMessagesFile: untranslatedMessagesFile, - usesNullableGetter: usesNullableGetter, - logger: _logger, - ) - ..loadResources() - ..writeOutputFiles(); - } on L10nException catch (e) { - throwToolExit(e.message); + // All other post processing. + if (format) { + if (outputFileList.isEmpty) { + return FlutterCommandResult.success(); + } + final String dartBinary = _artifacts.getHostArtifact(HostArtifact.engineDartBinary).path; + final List command = [dartBinary, 'format', ...outputFileList]; + final ProcessResult result = await _processManager.run(command); + if (result.exitCode != 0) { + throwToolExit('Formatting failed: $result', exitCode: result.exitCode); + } } return FlutterCommandResult.success(); diff --git a/packages/flutter_tools/lib/src/localizations/gen_l10n.dart b/packages/flutter_tools/lib/src/localizations/gen_l10n.dart index 208c2866de..e14fbdd17b 100644 --- a/packages/flutter_tools/lib/src/localizations/gen_l10n.dart +++ b/packages/flutter_tools/lib/src/localizations/gen_l10n.dart @@ -811,6 +811,10 @@ class LocalizationsGenerator { return _allBundles.bundles.map((AppResourceBundle bundle) => bundle.file.path).toList(); } + List get outputFileList { + return _outputFileList; + } + /// The supported language codes as found in the arb files located in /// [inputDirectory]. final Set supportedLanguageCodes = {}; @@ -1339,7 +1343,7 @@ class LocalizationsGenerator { || message.placeholdersRequireFormatting; }); - void writeOutputFiles({ bool isFromYaml = false }) { + List writeOutputFiles({ bool isFromYaml = false }) { // First, generate the string contents of all necessary files. final String generatedLocalizationsFile = _generateCode(); @@ -1372,9 +1376,7 @@ class LocalizationsGenerator { // Generate the required files for localizations. _languageFileMap.forEach((File file, String contents) { file.writeAsStringSync(contents); - if (inputsAndOutputsListFile != null) { - _outputFileList.add(file.absolute.path); - } + _outputFileList.add(file.absolute.path); }); baseOutputFile.writeAsStringSync(generatedLocalizationsFile); @@ -1407,9 +1409,8 @@ class LocalizationsGenerator { ); } final File? inputsAndOutputsListFileLocal = inputsAndOutputsListFile; + _outputFileList.add(baseOutputFile.absolute.path); if (inputsAndOutputsListFileLocal != null) { - _outputFileList.add(baseOutputFile.absolute.path); - // Generate a JSON file containing the inputs and outputs of the gen_l10n script. if (!inputsAndOutputsListFileLocal.existsSync()) { inputsAndOutputsListFileLocal.createSync(recursive: true); @@ -1422,14 +1423,14 @@ class LocalizationsGenerator { }), ); } + + return _outputFileList; } void _generateUntranslatedMessagesFile(Logger logger, File untranslatedMessagesFile) { if (_unimplementedMessages.isEmpty) { untranslatedMessagesFile.writeAsStringSync('{}'); - if (inputsAndOutputsListFile != null) { - _outputFileList.add(untranslatedMessagesFile.absolute.path); - } + _outputFileList.add(untranslatedMessagesFile.absolute.path); return; } @@ -1457,8 +1458,6 @@ class LocalizationsGenerator { resultingFile += '}\n'; untranslatedMessagesFile.writeAsStringSync(resultingFile); - if (inputsAndOutputsListFile != null) { - _outputFileList.add(untranslatedMessagesFile.absolute.path); - } + _outputFileList.add(untranslatedMessagesFile.absolute.path); } } diff --git a/packages/flutter_tools/lib/src/localizations/localizations_utils.dart b/packages/flutter_tools/lib/src/localizations/localizations_utils.dart index a8e4135266..d001c56026 100644 --- a/packages/flutter_tools/lib/src/localizations/localizations_utils.dart +++ b/packages/flutter_tools/lib/src/localizations/localizations_utils.dart @@ -311,6 +311,7 @@ class LocalizationOptions { this.useSyntheticPackage = true, this.areResourceAttributesRequired = false, this.usesNullableGetter = true, + this.format = false, }) : assert(useSyntheticPackage != null); /// The `--arb-dir` argument. @@ -377,6 +378,11 @@ class LocalizationOptions { /// /// Whether or not the localizations class getter is nullable. final bool usesNullableGetter; + + /// The `format` argument. + /// + /// Whether or not to format the generated files. + final bool format; } /// Parse the localizations configuration options from [file]. @@ -411,6 +417,7 @@ LocalizationOptions parseLocalizationsOptions({ useSyntheticPackage: _tryReadBool(yamlNode, 'synthetic-package', logger) ?? true, areResourceAttributesRequired: _tryReadBool(yamlNode, 'required-resource-attributes', logger) ?? false, usesNullableGetter: _tryReadBool(yamlNode, 'nullable-getter', logger) ?? true, + format: _tryReadBool(yamlNode, 'format', logger) ?? true, ); } diff --git a/packages/flutter_tools/test/commands.shard/hermetic/generate_localizations_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/generate_localizations_test.dart index 89d19d5328..85d7b917e3 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/generate_localizations_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/generate_localizations_test.dart @@ -5,19 +5,23 @@ // @dart = 2.8 import 'package:file/memory.dart'; +import 'package:flutter_tools/src/artifacts.dart'; import 'package:flutter_tools/src/base/file_system.dart'; import 'package:flutter_tools/src/base/logger.dart'; import 'package:flutter_tools/src/cache.dart'; import 'package:flutter_tools/src/commands/generate_localizations.dart'; -import 'package:flutter_tools/src/runner/flutter_command.dart'; import '../../integration.shard/test_data/basic_project.dart'; import '../../src/common.dart'; import '../../src/context.dart'; +import '../../src/fake_process_manager.dart'; import '../../src/test_flutter_command_runner.dart'; void main() { FileSystem fileSystem; + BufferLogger logger; + Artifacts artifacts; + FakeProcessManager processManager; setUpAll(() { Cache.disableLocking(); @@ -25,10 +29,12 @@ void main() { setUp(() { fileSystem = MemoryFileSystem.test(); + logger = BufferLogger.test(); + artifacts = Artifacts.test(); + processManager = FakeProcessManager.empty(); }); testUsingContext('default l10n settings', () async { - final BufferLogger logger = BufferLogger.test(); final File arbFile = fileSystem.file(fileSystem.path.join('lib', 'l10n', 'app_en.arb')) ..createSync(recursive: true); arbFile.writeAsStringSync(''' @@ -41,11 +47,11 @@ void main() { final GenerateLocalizationsCommand command = GenerateLocalizationsCommand( fileSystem: fileSystem, logger: logger, + artifacts: artifacts, + processManager: processManager, ); await createTestCommandRunner(command).run(['gen-l10n']); - final FlutterCommandResult result = await command.runCommand(); - expect(result.exitStatus, ExitStatus.success); final Directory outputDirectory = fileSystem.directory(fileSystem.path.join('.dart_tool', 'flutter_gen', 'gen_l10n')); expect(outputDirectory.existsSync(), true); expect(outputDirectory.childFile('app_localizations_en.dart').existsSync(), true); @@ -56,7 +62,6 @@ void main() { }); testUsingContext('not using synthetic packages', () async { - final BufferLogger logger = BufferLogger.test(); final Directory l10nDirectory = fileSystem.directory( fileSystem.path.join('lib', 'l10n'), ); @@ -75,14 +80,14 @@ void main() { final GenerateLocalizationsCommand command = GenerateLocalizationsCommand( fileSystem: fileSystem, logger: logger, + artifacts: artifacts, + processManager: processManager, ); await createTestCommandRunner(command).run([ 'gen-l10n', '--no-synthetic-package', ]); - final FlutterCommandResult result = await command.runCommand(); - expect(result.exitStatus, ExitStatus.success); expect(l10nDirectory.existsSync(), true); expect(l10nDirectory.childFile('app_localizations_en.dart').existsSync(), true); expect(l10nDirectory.childFile('app_localizations.dart').existsSync(), true); @@ -92,7 +97,6 @@ void main() { }); testUsingContext('throws error when arguments are invalid', () async { - final BufferLogger logger = BufferLogger.test(); final File arbFile = fileSystem.file(fileSystem.path.join('lib', 'l10n', 'app_en.arb')) ..createSync(recursive: true); arbFile.writeAsStringSync(''' @@ -107,6 +111,8 @@ void main() { final GenerateLocalizationsCommand command = GenerateLocalizationsCommand( fileSystem: fileSystem, logger: logger, + artifacts: artifacts, + processManager: processManager, ); expect( () => createTestCommandRunner(command).run([ @@ -122,7 +128,6 @@ void main() { }); testUsingContext('l10n yaml file takes precedence over command line arguments', () async { - final BufferLogger logger = BufferLogger.test(); final File arbFile = fileSystem.file(fileSystem.path.join('lib', 'l10n', 'app_en.arb')) ..createSync(recursive: true); arbFile.writeAsStringSync(''' @@ -138,11 +143,11 @@ void main() { final GenerateLocalizationsCommand command = GenerateLocalizationsCommand( fileSystem: fileSystem, logger: logger, + artifacts: artifacts, + processManager: processManager, ); await createTestCommandRunner(command).run(['gen-l10n']); - final FlutterCommandResult result = await command.runCommand(); - expect(result.exitStatus, ExitStatus.success); expect(logger.statusText, contains('Because l10n.yaml exists, the options defined there will be used instead.')); final Directory outputDirectory = fileSystem.directory(fileSystem.path.join('.dart_tool', 'flutter_gen', 'gen_l10n')); expect(outputDirectory.existsSync(), true); @@ -154,7 +159,6 @@ void main() { }); testUsingContext('nullable-getter help message is expected string', () async { - final BufferLogger logger = BufferLogger.test(); final File arbFile = fileSystem.file(fileSystem.path.join('lib', 'l10n', 'app_en.arb')) ..createSync(recursive: true); arbFile.writeAsStringSync(''' @@ -170,6 +174,8 @@ void main() { final GenerateLocalizationsCommand command = GenerateLocalizationsCommand( fileSystem: fileSystem, logger: logger, + artifacts: artifacts, + processManager: processManager, ); await createTestCommandRunner(command).run(['gen-l10n']); expect(command.usage, contains(' If this value is set to false, then ')); @@ -177,4 +183,88 @@ void main() { FileSystem: () => fileSystem, ProcessManager: () => FakeProcessManager.any(), }); + + testUsingContext('dart format is run when --format is passed', () async { + final File arbFile = fileSystem.file(fileSystem.path.join('lib', 'l10n', 'app_en.arb')) + ..createSync(recursive: true); + arbFile.writeAsStringSync(''' +{ + "helloWorld": "Hello, World!", + "@helloWorld": { + "description": "Sample description" + } +}'''); + processManager.addCommand( + const FakeCommand( + command: [ + 'HostArtifact.engineDartBinary', + 'format', + '/.dart_tool/flutter_gen/gen_l10n/app_localizations_en.dart', + '/.dart_tool/flutter_gen/gen_l10n/app_localizations.dart', + ] + ) + ); + + final GenerateLocalizationsCommand command = GenerateLocalizationsCommand( + fileSystem: fileSystem, + logger: logger, + artifacts: artifacts, + processManager: processManager, + ); + + await createTestCommandRunner(command).run(['gen-l10n', '--format']); + + final Directory outputDirectory = fileSystem.directory(fileSystem.path.join('.dart_tool', 'flutter_gen', 'gen_l10n')); + expect(outputDirectory.existsSync(), true); + expect(outputDirectory.childFile('app_localizations_en.dart').existsSync(), true); + expect(outputDirectory.childFile('app_localizations.dart').existsSync(), true); + expect(processManager, hasNoRemainingExpectations); + }, overrides: { + FileSystem: () => fileSystem, + ProcessManager: () => FakeProcessManager.any(), + }); + + testUsingContext('dart format is run when format: true is passed into l10n.yaml', () async { + final File arbFile = fileSystem.file(fileSystem.path.join('lib', 'l10n', 'app_en.arb')) + ..createSync(recursive: true); + arbFile.writeAsStringSync(''' +{ + "helloWorld": "Hello, World!", + "@helloWorld": { + "description": "Sample description" + } +}'''); + final File configFile = fileSystem.file('l10n.yaml')..createSync(); + configFile.writeAsStringSync(''' +format: true +'''); + final File pubspecFile = fileSystem.file('pubspec.yaml')..createSync(); + pubspecFile.writeAsStringSync(BasicProjectWithFlutterGen().pubspec); + processManager.addCommand( + const FakeCommand( + command: [ + 'HostArtifact.engineDartBinary', + 'format', + '/.dart_tool/flutter_gen/gen_l10n/app_localizations_en.dart', + '/.dart_tool/flutter_gen/gen_l10n/app_localizations.dart', + ] + ) + ); + final GenerateLocalizationsCommand command = GenerateLocalizationsCommand( + fileSystem: fileSystem, + logger: logger, + artifacts: artifacts, + processManager: processManager, + ); + await createTestCommandRunner(command).run(['gen-l10n']); + + final Directory outputDirectory = fileSystem.directory(fileSystem.path.join('.dart_tool', 'flutter_gen', 'gen_l10n')); + expect(outputDirectory.existsSync(), true); + expect(outputDirectory.childFile('app_localizations_en.dart').existsSync(), true); + expect(outputDirectory.childFile('app_localizations.dart').existsSync(), true); + expect(processManager, hasNoRemainingExpectations); + }, overrides: { + FileSystem: () => fileSystem, + ProcessManager: () => FakeProcessManager.any(), + }); }