intl: Migrate from `package:flutter_gen`; include generated files #1058

rajveermalviya · 2024-11-13T15:50:47Z

flutter tool deprecated the package:flutter_gen synthetic package:
https://flutter.dev/to/flutter-gen-deprecation

The generated localization source files are now included in the
repository and referenced directly.

Without this change, flutter run generates the following warning:

Synthetic package output (package:flutter_gen) is deprecated:
https://flutter.dev/to/flutter-gen-deprecation. In a future
release, synthetic-package will default to `false` and will
later be removed entirely.

PIG208

Thanks for the PR. Looks good to me overall! Left some small comments.

PIG208 · 2024-11-15T21:03:41Z

lib/model/localizations.dart

@@ -1,4 +1,4 @@
-import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
+import '/generated/l10n/zulip_localizations.dart';


Should we use relative path like we do in the other files, or is it needed for other reasons?

PIG208 · 2024-11-15T21:19:56Z

tools/check

@@ -241,6 +241,19 @@ should_run_build_runner() {
    return 1
 }

+run_l10n() {
+    files_check lib/generated/l10n/'*'.dart \


nit: like

local schema_dir=test/model/schemas/

we can extract a local variable for the output path

rajveermalviya · 2024-11-15T21:58:15Z

Thanks for the review @PIG208! Pushed a new revision.

CI is failing because Flutter upstream changed the generated header for localization files in flutter/flutter@dca37ad, resulting in following diff:

diff --git a/lib/generated/l10n/zulip_localizations_ar.dart b/lib/generated/l10n/zulip_localizations_ar.dart
index fedc8b74..b7c1ed6d 100644
--- a/lib/generated/l10n/zulip_localizations_ar.dart
+++ b/lib/generated/l10n/zulip_localizations_ar.dart
@@ -1,5 +1,5 @@
+// ignore: unused_import
 import 'package:intl/intl.dart' as intl;
-
 import 'zulip_localizations.dart';
 
 // ignore_for_file: type=lint
…
…

Added a head commit to update Flutter, and regenerated the files, that should fix the CI now.

PIG208 · 2024-11-15T22:43:03Z

LGTM! Tested on Android and it works for me.

gnprice

Thanks @rajveermalviya for taking care of this! And thanks @PIG208 for the previous review (and @chrisbobbe for the review on the predecessor PR #1057).

Generally this all looks great; a few comments below. All nits except there's one logic error in the new tools/check l10n suite.

gnprice · 2024-11-19T04:59:50Z

.gitattributes

+# Generated file for localizations.
+lib/generated/l10n/*.dart -diff


nit: plural because it's several files

Suggested change

# Generated file for localizations.

lib/generated/l10n/*.dart -diff

# Generated files for localizations.

lib/generated/l10n/*.dart -diff

gnprice · 2024-11-19T05:03:02Z

docs/translation.md

@@ -54,12 +54,8 @@ changes to the ARB file.

 ### Using a translated string in the code

-To use in our widgets, you need to import the generated bindings:
-```
-import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';


super nit: the main migration commit should update this line, because otherwise the instruction is incorrect as of that commit

(then the later commit can delete, like in this revision, along with that nice commit message explaining why it used to be needed and no longer is)

-import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; +import '/generated/l10n/zulip_localizations.dart';

If this nit were the only thing remaining in this revision I'd probably just go ahead and merge it, but since we have another round anyway: this (temporary) advice should match what we actually do in the source files 🙂, which follows #1058 (comment) .

gnprice · 2024-11-19T05:05:00Z

tools/check

@@ -241,6 +241,21 @@ should_run_build_runner() {
    return 1
 }

+run_l10n() {


nit: move this above should_run_build_runner, which is a helper function specific to run_build_runner, so as not to split those from each other

(alternatively, move this suite after the build_runner suite)

gnprice · 2024-11-19T05:12:26Z

tools/check

+    local output_path=lib/generated/l10n
+
+    files_check "${output_path}"/'*'.dart \


nit: pull the whole pathspec lib/generated/l10n/'*'.dart out as a local variable

That way there's a natural place for a comment clarifying that this string is meant as a Git pathspec. Otherwise it looks an awful lot like a shell glob (which it would be if the * weren't escaped with those single quotes), which would have somewhat different semantics. See run_pigeon for an example (though that example also makes it an array, which isn't needed here since there's just one pathspec).

gnprice · 2024-11-19T05:14:18Z

tools/check

+    local output_path=lib/generated/l10n
+
+    files_check "${output_path}"/'*'.dart \


I think this check is too narrow and will miss some cases where this suite should run.

For examples, look back at the other files_check call sites.

This should also include an "Omitted from this check" comment like the ones at those other call sites, listing all the other files in the repo that would belong in a 100% version of this check (because changes to them could affect whether this suite passes) but which we're choosing to leave out as described at the files_check doc comment. I put those there partly as a way of making sure I explicitly thought through what files could matter to each check, to try to avoid this class of bug 🙂

This pulls in: flutter/flutter@dca37ad which slightly changed the generated header for localization files.

rajveermalviya · 2024-11-19T06:40:09Z

Thanks for the review @gnprice. Pushed a new revision, PTAL.

gnprice

Thanks for the revision! Comments below.

gnprice · 2024-11-19T18:32:54Z

tools/check

+    # Omitted from this check:
+    #   pubspec.{yaml,lock} tools/check
+    files_check assets/l10n/ "${output_files}" \


What about l10n.yaml?

gnprice · 2024-11-19T18:38:14Z

tools/check

@@ -277,6 +277,23 @@ run_build_runner() {
    check_no_changes "updates to *.g.dart files" '*.g.dart'
 }

+run_l10n() {
+    local output_files=lib/generated/l10n/zulip_localizations'*'.dart


nit: I don't love calling this "files" — it makes it sound like it's an actual path (or list of actual paths) to some files. As mentioned at #1058 (comment) , I'd like to make explicit that this is instead a Git pathspec.

This would do it:

Suggested change

local output_files=lib/generated/l10n/zulip_localizations'*'.dart

local output_pathspec=lib/generated/l10n/zulip_localizations'*'.dart

gnprice · 2024-11-19T18:40:07Z

docs/translation.md

@@ -54,12 +54,8 @@ changes to the ARB file.

 ### Using a translated string in the code

-To use in our widgets, you need to import the generated bindings:
-```
-import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';


-import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; +import '/generated/l10n/zulip_localizations.dart';

If this nit were the only thing remaining in this revision I'd probably just go ahead and merge it, but since we have another round anyway: this (temporary) advice should match what we actually do in the source files 🙂, which follows #1058 (comment) .

`flutter` tool deprecated the `package:flutter_gen` synthetic package: https://flutter.dev/to/flutter-gen-deprecation The generated localization source files are now included in the repository and referenced directly. Without this change, `flutter run` generates the following warning: Synthetic package output (package:flutter_gen) is deprecated: https://flutter.dev/to/flutter-gen-deprecation. In a future release, synthetic-package will default to `false` and will later be removed entirely.

Previously with `package:flutter_gen`, a manual import of the bindings file was required because editor/IDE plugins didn't automatically suggest imports for the `ZulipLocalizations` class. See: https://github.com/dart-lang/linter/issues/ 3308 https://github.com/Dart-Code/Dart-Code/issues/ 3948 https://github.com/flutter/flutter-intellij/issues/ 4869 Now that the generated files are located in `lib/generated/l10n`, IDE plugins provide auto-import suggestions, making the manual import instruction unnecessary.

This package is no longer used.

rajveermalviya · 2024-11-19T21:13:29Z

Thanks for the review @gnprice! Pushed a new revision, PTAL.

gnprice · 2024-11-19T21:21:52Z

Thanks! Looks good; merging.

gnprice · 2024-11-20T02:06:40Z

Hmmm, the commits I merged earlier today looked like this:
ae8aabc deps: Upgrade Flutter to 3.27.0-1.0.pre.518
661adfc intl: Migrate from package:flutter_gen; include generated files
8293e81 (TODO fix links) intl docs: Remove instruction to import bindings file
c6a2960 check: Remove workaround for package:flutter_gen
d03ec1e check: Add suite l10n

Spot the error 🙂

Nothing else has been merged since then, and it looks like there aren't even any PRs that have been updated since this was merged. So I think it's not too late to fix it; I'll go and do so.

gnprice · 2024-11-20T02:10:31Z

OK, done. Now the corresponding commits in main are:
ae8aabc deps: Upgrade Flutter to 3.27.0-1.0.pre.518
661adfc intl: Migrate from package:flutter_gen; include generated files
c15bc77 intl docs: Remove instruction to import bindings file
8148aa4 check: Remove workaround for package:flutter_gen
c6c2f33 check: Add suite l10n

(If it were just the broken links themselves, I'd leave them there; the reader who wants to follow them always could, just with a small amount of annoyance. But the big capital-letters "TODO" right in the summary line would have kept bugging me into the future when looking at the history. 🙂)

rajveermalviya mentioned this pull request Nov 13, 2024

deps: Upgrade Flutter; and handle migrations #1057

Merged

rajveermalviya marked this pull request as draft November 15, 2024 14:00

rajveermalviya marked this pull request as ready for review November 15, 2024 20:27

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Nov 15, 2024

rajveermalviya requested a review from PIG208 November 15, 2024 20:27

rajveermalviya assigned PIG208 Nov 15, 2024

rajveermalviya force-pushed the pr-intl-handle-migration branch 2 times, most recently from 4fc031a to cc0ceec Compare November 15, 2024 20:42

PIG208 reviewed Nov 15, 2024

View reviewed changes

rajveermalviya force-pushed the pr-intl-handle-migration branch 2 times, most recently from ead0ef8 to a3bccfd Compare November 15, 2024 21:54

rajveermalviya force-pushed the pr-intl-handle-migration branch from a3bccfd to 6758fd5 Compare November 15, 2024 22:03

rajveermalviya requested a review from PIG208 November 15, 2024 22:08

PIG208 removed the maintainer review PR ready for review by Zulip maintainers label Nov 15, 2024

PIG208 removed their assignment Nov 15, 2024

PIG208 requested review from gnprice and removed request for PIG208 November 15, 2024 22:40

PIG208 added the integration review Added by maintainers when PR may be ready for integration label Nov 15, 2024

PIG208 assigned gnprice Nov 15, 2024

gnprice reviewed Nov 19, 2024

View reviewed changes

rajveermalviya force-pushed the pr-intl-handle-migration branch from 6758fd5 to b04510c Compare November 19, 2024 06:36

deps: Upgrade Flutter to 3.27.0-1.0.pre.518

ae8aabc

This pulls in: flutter/flutter@dca37ad which slightly changed the generated header for localization files.

rajveermalviya force-pushed the pr-intl-handle-migration branch from b04510c to f21a59c Compare November 19, 2024 06:37

rajveermalviya requested a review from gnprice November 19, 2024 06:40

gnprice reviewed Nov 19, 2024

View reviewed changes

rajveermalviya added 2 commits November 20, 2024 02:10

rajveermalviya added 2 commits November 20, 2024 02:11

check: Remove workaround for package:flutter_gen

c6a2960

This package is no longer used.

check: Add suite l10n

d03ec1e

rajveermalviya force-pushed the pr-intl-handle-migration branch from f21a59c to d03ec1e Compare November 19, 2024 21:07

rajveermalviya requested a review from gnprice November 19, 2024 21:13

gnprice merged commit d03ec1e into zulip:main Nov 19, 2024
1 check passed

rajveermalviya deleted the pr-intl-handle-migration branch November 21, 2024 04:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

intl: Migrate from `package:flutter_gen`; include generated files #1058

intl: Migrate from `package:flutter_gen`; include generated files #1058

rajveermalviya commented Nov 13, 2024

PIG208 left a comment

PIG208 Nov 15, 2024

PIG208 Nov 15, 2024

rajveermalviya commented Nov 15, 2024

PIG208 commented Nov 15, 2024

gnprice left a comment

gnprice Nov 19, 2024

gnprice Nov 19, 2024

gnprice Nov 19, 2024

gnprice Nov 19, 2024

gnprice Nov 19, 2024

gnprice Nov 19, 2024

rajveermalviya commented Nov 19, 2024

gnprice left a comment

gnprice Nov 19, 2024

gnprice Nov 19, 2024

gnprice Nov 19, 2024

rajveermalviya commented Nov 19, 2024

gnprice commented Nov 19, 2024

gnprice commented Nov 20, 2024

gnprice commented Nov 20, 2024

		@@ -1,4 +1,4 @@
		import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
		import '/generated/l10n/zulip_localizations.dart';

		# Generated file for localizations.
		lib/generated/l10n/*.dart -diff

		local output_path=lib/generated/l10n

		files_check "${output_path}"/'*'.dart \

	local output_files=lib/generated/l10n/zulip_localizations'*'.dart
	local output_pathspec=lib/generated/l10n/zulip_localizations'*'.dart

intl: Migrate from package:flutter_gen; include generated files #1058

intl: Migrate from package:flutter_gen; include generated files #1058

Conversation

rajveermalviya commented Nov 13, 2024

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Nov 15, 2024

PIG208 commented Nov 15, 2024

gnprice 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

rajveermalviya commented Nov 19, 2024

gnprice 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

rajveermalviya commented Nov 19, 2024

gnprice commented Nov 19, 2024

gnprice commented Nov 20, 2024

gnprice commented Nov 20, 2024

intl: Migrate from `package:flutter_gen`; include generated files #1058

intl: Migrate from `package:flutter_gen`; include generated files #1058