[V4]: Migrate to EasyLocalization v4 with improved initialization, asset loading, and storage #742
Conversation
…pported locales data and setting the device and saved locale.
WalkthroughThe pull request introduces significant updates to the Easy Localization package, focusing on enhancing localization asset loading, storage management, and internationalization support. The changes include modifications to method signatures, asset loading mechanisms, and test infrastructure. Key updates involve introducing a new storage interface, updating asset loader functionality, expanding supported locales, and refactoring the localization controller to provide more flexible locale management. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
example/lib/lang_view.dart (1)
Line range hint
39-43: Avoid hardcoded indices for locale selection.Using hardcoded indices (
context.supportedLocales[1]) is fragile and could break if the locale order changes.Consider using locale codes instead:
- locale: context.supportedLocales[1] //BuildContext extension method + locale: context.supportedLocales.firstWhere( + (l) => l.languageCode == 'ar', + orElse: () => context.supportedLocales.first, + )
🧹 Nitpick comments (16)
lib/src/easy_localization_controller.dart (4)
3-3: Remove or clarify commented-out import
The newly added imports for path utilities, asset loader, and storage interface align with the refactor. However, the commented-outSharedPreferencesimport may cause confusion if left unused. Consider removing it to keep the codebase clean.Also applies to: 8-8, 11-13
18-19: Revisit mutability of_storage
If the storage implementation isn’t meant to change at runtime, consider making_storagealate finalto prevent accidental reassignment. This clarifies intent and may reduce the chance of concurrency issues.
154-158: Remove lingeringSharedPreferencesreferences
The commented-out code suggests a previous approach to locale persistence. Since storage is now handled via_storage, removing these lines can reduce clutter and confusion.
189-240: Clarify asset-based loading for cross-platform usage
- Reading locales from a directory via
dart:iois incompatible with platforms like web. Ensure that this path-based approach is either gated by platform checks or supplemented with a fallback.- Throwing
io.PathExistsExceptionmight be misleading; a custom exception or a more descriptive message can improve debugging.lib/src/easy_localization_storage_interface.dart (1)
1-19: Add typed getters to avoid runtime type errors
Using a generic methodgetValue<T>()is flexible but may introduce runtime type mismatches. Consider providing typed methods (e.g.,getString()) or robust runtime checks. Also ensureclose()is reliably invoked to release resources (if needed).test/root_bundle_asset_loader_test.dart (1)
5-6: Remove TODO comment.The comment "Replace with your package and file name" should be removed as it's no longer needed.
test/easy_localization_ctl_test.dart (1)
7-15: Consider using a mock logger instead of print override.While the print override works, using a mock logger would be more maintainable and allow for better verification of log messages.
Consider this approach:
class MockLogger { final List<String> logs = []; void debug(String message) => logs.add(message); } final mockLogger = MockLogger(); // Inject mockLogger into EasyLocalizationControllerexample/lib/lang_view.dart (1)
101-105: Add semantic labels for accessibility.The language selection tiles should have semantic labels for better accessibility.
Add semantics to the ListTile:
child: ListTile( dense: true, + semanticLabel: 'Select $title language', title: Text( title, ),example/lib/main.dart (2)
26-35: Consider removing commented-out configuration options.The commented options for
fallbackLocaleandstartLocaleshould either be implemented or removed to maintain clean code.
41-41: Consider removing debug print statement.The debug print statement for supported locales should be removed before production deployment.
lib/src/easy_localization_app.dart (1)
104-109: Remove commented-out code.The commented-out properties should be removed as they're now handled by the controller.
test/easy_localization_context_test.dart (1)
43-44: Consider adding error handling test cases for asset loader initialization.The initialization of EasyLocalization with RootBundleAssetLoader should include test cases for error scenarios, such as invalid paths or missing assets.
await EasyLocalization.ensureInitialized( - assetLoader: const RootBundleAssetLoader('i18n')); + assetLoader: const RootBundleAssetLoader('i18n')).catchError((error) { + expect(error, isA<AssetLoaderException>()); + });test/easy_localization_test.dart (1)
75-85: Remove commented code to improve readability.The commented parameters are no longer needed with the new initialization pattern.
var r1 = EasyLocalizationController( forceLocale: const Locale('en'), - // path: 'path/en.json', - // supportedLocales: const [Locale('en')], useOnlyLangCode: true, useFallbackTranslations: false, saveLocale: false, onLoadError: (FlutterError e) { log(e.toString()); }, - // assetLoader: const JsonAssetLoader()test/easy_localization_widget_test.dart (1)
71-78: Consider enabling logger levels for debugging.The commented logger levels could be useful for debugging. Consider keeping them enabled in test environment.
TestWidgetsFlutterBinding.ensureInitialized(); SharedPreferences.setMockInitialValues({}); - // EasyLocalization.logger.enableLevels = <LevelMessages>[ - // LevelMessages.error, - // LevelMessages.warning, - // ]; + EasyLocalization.logger.enableLevels = <LevelMessages>[ + LevelMessages.error, + LevelMessages.warning, + ];i18n/ar/ar.json (1)
1-11: Add metadata and more comprehensive translations.Consider enhancing the JSON file with:
- Metadata about the language and region
- More comprehensive test cases
- Documentation for pluralization rules
{ + "_metadata": { + "language": "Arabic", + "country": "General", + "pluralization_rules": "Arabic has six plural forms" + }, "test": "اختبار", "day": { "zero": "{} يوم", "one": "{} يوم", "two": "{} أيام", "few": "{} أيام", "many": "{} يوم", "other": "{} يوم" } }example/pubspec.yaml (1)
Line range hint
31-34: Address the commented loader package dependency issue.There's a FIXME comment regarding the
easy_localization_loaderpackage dependencies issue that needs to be resolved.Would you like me to help investigate and resolve the dependency issue with the
easy_localization_loaderpackage? I can help create an issue to track this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
example/lib/generated/codegen_loader.g.dartis excluded by!**/generated/**
📒 Files selected for processing (21)
bin/generate.dart(1 hunks)example/lib/lang_view.dart(3 hunks)example/lib/main.dart(3 hunks)example/linux/flutter/CMakeLists.txt(1 hunks)example/pubspec.yaml(2 hunks)example/test/widget_test.dart(1 hunks)i18n/ar/ar.json(1 hunks)i18n/ar_en/ar.json(1 hunks)i18n/ar_en/en.json(1 hunks)lib/easy_localization.dart(1 hunks)lib/src/asset_loader.dart(1 hunks)lib/src/easy_localization_app.dart(7 hunks)lib/src/easy_localization_controller.dart(4 hunks)lib/src/easy_localization_storage_interface.dart(1 hunks)pubspec.yaml(1 hunks)test/easy_localization_context_test.dart(1 hunks)test/easy_localization_ctl_test.dart(1 hunks)test/easy_localization_test.dart(7 hunks)test/easy_localization_widget_test.dart(9 hunks)test/root_bundle_asset_loader_test.dart(1 hunks)test/utils/test_asset_loaders.dart(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- example/test/widget_test.dart
- i18n/ar_en/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (25)
lib/src/easy_localization_controller.dart (5)
1-2: Ensure platform compatibility when usingdart:io
While this import is necessary for reading directories on platforms that support I/O, be aware that web builds don’t fully supportdart:io. If web compatibility is intended, consider a conditional import or alternative approach.
26-29: Initializelatefields thoroughly
Ensure_assetLoaderand_localeare always assigned during initialization. If the controller initialization is skipped in certain conditions (e.g., testing), referencing these fields prematurely can throw runtime errors.
102-117: Validate fallback translation flow
When loading fallback translations, the code merges base and fallback data. This approach appears correct, but keep in mind that_listtranslationDatais purged after its first use, forcing subsequent calls to rely solely on_assetLoader. Verify that this matches your intended behavior for multi-locale or repeated fallback scenarios.
162-185: Confirm one-time vs. repeatedinitEasyLocationcalls
This static method sets_storageand_assetLoaderglobally. IfinitEasyLocationcan be invoked multiple times, ensure that repeated calls won’t overwrite existing values or inadvertently reset partially loaded data.
241-241: Locale deletion confirmed
Removing the saved locale updates both the in-memory state and_storage. If an application flow requires resetting to a fallback or re-initializing after deletion, consider prompting re-initialization or reloading to avoid unexpected locale states.Also applies to: 244-244
lib/easy_localization.dart (1)
7-7: Export interface for custom storage
Exportingeasy_localization_storage_interface.dartallows external code to implement custom storage strategies, extending the library’s flexibility without tying users to a single persistence layer.test/easy_localization_ctl_test.dart (1)
50-51: Add test cases for core functionality.The TODO comment indicates missing test cases. Based on the PR objectives, tests should be added for:
- Asset loading initialization
- Storage interface integration
- Cached asset loader functionality
Let me help verify what needs to be tested:
✅ Verification successful
Add test cases for core functionality
The review comment is accurate. The codebase shows several untested core components:
- Asset Loaders:
- CodegenLoader
- ImmutableAssetLoader
- ImmutableJsonAssetLoader
- JsonAssetLoader
- Storage interface implementation and integration
- EasyLocalizationController functionality (currently has commented-out test cases)
The existing test coverage only covers RootBundleAssetLoader and basic Localization.load() scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find core functionality that needs test coverage echo "Searching for core functionality..." rg -t dart "class.*Storage|class.*AssetLoader|class.*Controller" --no-filename # Find existing test coverage echo -e "\nSearching for existing tests..." rg -t dart "^.*test.*Storage|^.*test.*AssetLoader|^.*test.*Controller" --no-filenameLength of output: 840
Script:
#!/bin/bash # Find all test files echo "All test files:" fd -e dart "_test.dart$" echo -e "\nController implementation:" ast-grep --pattern 'class EasyLocalizationController { $$$ }' echo -e "\nExisting test cases (broader search):" rg -t dart "test.*\b(Storage|AssetLoader|Controller|load|save|init)" -A 2Length of output: 2722
test/utils/test_asset_loaders.dart (3)
5-14: LGTM! Well-structured test utility class.The
ImmutableAssetLoaderprovides a clean, minimal implementation suitable for testing scenarios.
17-25: LGTM! Constructor and method signature updates align with new interface.The changes properly implement the new AssetLoader interface while maintaining the test utility's immutable nature.
Line range hint
29-103: LGTM! Comprehensive test data with proper null safety.The changes properly implement the new AssetLoader interface while maintaining a robust test data structure. The null-safe access to
locale?.languageCodeis a good practice.example/lib/main.dart (2)
Line range hint
11-24: LGTM! Proper initialization setup with asset loader configuration.The initialization is correctly configured with the new
ensureInitializedmethod, providing a clear path for asset loading.
47-47: LGTM! Material 3 adoption.Good practice to adopt Material 3 for modern UI components and features.
lib/src/easy_localization_app.dart (3)
65-67: Verify impact of changing saveLocale default to false.This change might affect existing applications that rely on locale persistence. Consider documenting this breaking change in migration guide.
82-87: LGTM! Well-structured initialization method.The
ensureInitializedmethod properly handles both assetLoader and storage initialization.
171-172: LGTM! Proper centralization of locale management.Good practice to centralize supported locales in the controller and validate against it.
Also applies to: 194-194
bin/generate.dart (1)
247-247: LGTM! Method signature properly updated for null safety.The
loadmethod correctly implements the new AssetLoader interface with proper null safety.test/easy_localization_context_test.dart (2)
66-236: 🛠️ Refactor suggestionConsider keeping essential test cases from commented code.
While removing SharedPreferences dependency is good, some of the commented test cases cover important functionality like fallback locale and device locale handling. Consider preserving these tests with the new storage interface.
60-61: Verify test coverage for supported locales.The test only checks the length of supported locales but not their specific values. Consider adding more detailed assertions.
expect(_context.supportedLocales.length, 4); +expect(_context.supportedLocales, containsAll([ + const Locale("ar", "DZ"), + const Locale("en"), + const Locale("ar"), + const Locale("en", "US") +]));✅ Verification successful
The review suggestion is accurate and should be implemented
The suggested assertion precisely matches the actual supported locales found in the i18n directory:
- ar-DZ
- ar
- en
- en-US
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # First, let's look at the test file content cat test/easy_localization_context_test.dartLength of output: 8339
Script:
#!/bin/bash # Look for locale definitions in the source code rg -l "supportedLocales" --type dart # Also check for any i18n configuration files fd "i18n"Length of output: 414
Script:
#!/bin/bash # Check i18n directory content ls -la i18n/ # Look at the initialization code in the main implementation file cat lib/src/easy_localization_app.dartLength of output: 7980
test/easy_localization_test.dart (1)
26-48: LGTM! Good addition of asset loader test cases.The new test group for asset loader properly verifies error cases for path and supported locales.
i18n/ar_en/ar.json (1)
1-11: Verify Arabic pluralization rules and placeholder consistency.The JSON structure looks good, but there are a few points to consider:
- The placeholder
{}is consistently used across all plural forms- The plural forms follow Arabic grammar rules with appropriate forms for zero, one, two, few, many, and other cases
Let's validate the JSON structure and encoding:
✅ Verification successful
Arabic pluralization rules and placeholders are correctly implemented ✅
The plural forms and placeholders are consistent across all Arabic translation files and follow the correct Arabic grammar rules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify JSON validity and UTF-8 encoding file="i18n/ar_en/ar.json" if [ -f "$file" ]; then # Check JSON syntax jq '.' "$file" > /dev/null # Verify UTF-8 encoding file -i "$file" | grep -q "charset=utf-8" # Print the normalized content for visual inspection jq -r '.' "$file" fiLength of output: 430
Script:
#!/bin/bash # Check for other Arabic translation files and references echo "Looking for other Arabic translation files:" fd -e json . i18n | grep -i "ar" echo -e "\nChecking for files referencing Arabic translations:" rg -l "ar\.json" --type ts --type jsLength of output: 519
Script:
#!/bin/bash # Compare plural rules across Arabic translation files echo "=== Standard Arabic (ar.json) ===" jq -r '.day | select(.)' i18n/ar.json 2>/dev/null || echo "No 'day' key found" echo -e "\n=== Algerian Arabic (ar-DZ.json) ===" jq -r '.day | select(.)' i18n/ar-DZ.json 2>/dev/null || echo "No 'day' key found" echo -e "\n=== Arabic in ar/ directory ===" jq -r '.day | select(.)' i18n/ar/ar.json 2>/dev/null || echo "No 'day' key found" echo -e "\n=== Arabic in ar_en/ directory (current file) ===" jq -r '.day | select(.)' i18n/ar_en/ar.json 2>/dev/null || echo "No 'day' key found"Length of output: 1023
pubspec.yaml (3)
8-8: Version bump indicates breaking changes.The version change to
4.0.0-dev.0correctly follows semantic versioning for breaking changes, as indicated in the PR objectives regarding initialization changes.
51-55: Asset paths align with new localization structure.The flutter assets section correctly includes the new i18n directory structure, which supports the improved asset loading mechanism mentioned in the PR objectives.
33-33: Verify compatibility impact of SDK constraint.The SDK constraint has been updated to require Dart
>=3.1.0. This is a significant change that might affect existing users.✅ Verification successful
SDK constraint change aligns with existing Dart 3 support ✅
The package already supports Dart 3 since version 3.0.2 as documented in the changelog, making this constraint change a formalization of existing compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check pub points and SDK compatibility curl -s "https://pub.dev/api/packages/easy_localization" | \ jq -r '.versions[] | select(.version | contains("3.")) | .pubspec.environment.sdk' | \ sort -V | tail -n 1Length of output: 179
Script:
#!/bin/bash # Check for migration guides and changelog entries echo "=== Checking for CHANGELOG.md ===" if [ -f CHANGELOG.md ]; then head -n 20 CHANGELOG.md fi echo -e "\n=== Checking for migration guides ===" fd -t f -e md migration fd -t f -e md guide echo -e "\n=== Checking for Dart 3.0 related changes ===" rg -i "dart 3" -A 2 -B 2Length of output: 942
example/linux/flutter/CMakeLists.txt (1)
85-85: LGTM! Platform-agnostic build configuration.The change to use
${FLUTTER_TARGET_PLATFORM}makes the build configuration more flexible and platform-agnostic.example/pubspec.yaml (1)
52-54: Resource paths support multiple localization formats.The addition of paths for JSON, YML, and XML formats aligns with the improved asset loading mechanism. However, ensure that the corresponding loader implementations are available and tested.
Consider documenting the supported formats and their usage in the migration guide.
| static late List<Map<String, dynamic>>? _listtranslationData; | ||
|
|
||
| static List<Locale> _supportedLocales = []; | ||
|
|
||
| static List<Locale> get supportedLocales => _supportedLocales; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid static storage for ephemeral data
_listtranslationData is cleared after a single usage. Having it as a static variable can cause confusion or unexpected behavior if multiple instances or dynamic re-initializations occur. Consider a more localized data structure or a dedicated caching solution.
| required this.useOnlyLangCode, | ||
| required this.onLoadError, | ||
| Locale? startLocale, | ||
| Locale? fallbackLocale, | ||
| List<Locale>? supportedLocales, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce consistent handling of _supportedLocales
- The assertion requiring non-null storage if
saveLocaleis true is a good safeguard against misconfiguration. - If
_supportedLocalesis intended to be passed into the controller, consider removing or uncommenting line 52 to avoid confusion. - Confirm that
_supportedLocalesis fully populated prior to referencing it for fallback logic, especially ifinitEasyLocationis called after constructor usage.
Also applies to: 48-50, 52-52, 57-57, 64-64, 71-71
| late final Map<String, dynamic> translationData; | ||
|
|
||
| if (useOnlyLangCode) { | ||
| data = await assetLoader.load(path, Locale(locale.languageCode)); | ||
| } else { | ||
| data = await assetLoader.load(path, locale); | ||
| try { | ||
| if (_listtranslationData != null) { | ||
| var index = _supportedLocales.indexOf(locale); | ||
| translationData = _listtranslationData![index]; | ||
| _listtranslationData?.clear(); | ||
| _listtranslationData = null; | ||
| } else { | ||
| translationData = await _assetLoader.load(locale: locale); | ||
| } | ||
| } catch (e) { | ||
| EasyLocalization.logger.error('loadTranslationData $e '); | ||
| } | ||
|
|
||
| if (data == null) return {}; | ||
|
|
||
| return data; | ||
| return translationData; |
There was a problem hiding this comment.
Handle indexOf result and repeated loads
- If
_supportedLocales.indexOf(locale)returns -1, accessing_listtranslationData![index]will throw an error. Consider a safe check or a default handler for unknown locales. - Clearing
_listtranslationDataat first use prevents subsequent reuse. If fallback locales or repeated loading are required, reevaluate whether this data should remain available.
| /// Abstract class for loading assets. | ||
| abstract class AssetLoader { | ||
| const AssetLoader(); | ||
| Future<Map<String, dynamic>?> load(String path, Locale locale); | ||
| /// Path to the assets directory. | ||
| /// Example: | ||
| /// ```dart | ||
| /// path: 'assets/translations', | ||
| /// path: 'assets/translations/lang.csv', | ||
| /// ``` | ||
| final String? path; | ||
|
|
||
| /// List of supported locales. | ||
| /// {@macro flutter.widgets.widgetsApp.supportedLocales} | ||
| final List<Locale>? supportedLocales; | ||
|
|
||
| /// Constructor for [AssetLoader]. | ||
| /// | ||
| /// [path] is the path to the assets directory. | ||
| /// [supportedLocales] is a list of locales that the assets support. | ||
| const AssetLoader({this.path, this.supportedLocales}) | ||
| : assert(path != null || supportedLocales != null, | ||
| 'path or supportedLocales must not be null'); | ||
|
|
||
| /// Loads the assets for the given [locale]. | ||
| /// | ||
| /// Returns a map of loaded assets. | ||
| Future<Map<String, dynamic>> load({Locale? locale}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making locale required in load() method.
While the class structure and documentation look good, making locale optional in the load() method seems inconsistent since implementations like RootBundleAssetLoader require it and throw an error if it's null.
Consider updating the method signature to:
- Future<Map<String, dynamic>> load({Locale? locale});
+ Future<Map<String, dynamic>> load({required Locale locale});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Abstract class for loading assets. | |
| abstract class AssetLoader { | |
| const AssetLoader(); | |
| Future<Map<String, dynamic>?> load(String path, Locale locale); | |
| /// Path to the assets directory. | |
| /// Example: | |
| /// ```dart | |
| /// path: 'assets/translations', | |
| /// path: 'assets/translations/lang.csv', | |
| /// ``` | |
| final String? path; | |
| /// List of supported locales. | |
| /// {@macro flutter.widgets.widgetsApp.supportedLocales} | |
| final List<Locale>? supportedLocales; | |
| /// Constructor for [AssetLoader]. | |
| /// | |
| /// [path] is the path to the assets directory. | |
| /// [supportedLocales] is a list of locales that the assets support. | |
| const AssetLoader({this.path, this.supportedLocales}) | |
| : assert(path != null || supportedLocales != null, | |
| 'path or supportedLocales must not be null'); | |
| /// Loads the assets for the given [locale]. | |
| /// | |
| /// Returns a map of loaded assets. | |
| Future<Map<String, dynamic>> load({Locale? locale}); | |
| /// Abstract class for loading assets. | |
| abstract class AssetLoader { | |
| /// Path to the assets directory. | |
| /// Example: |
| /// The `RootBundleAssetLoader` class is a subclass of `AssetLoader` that uses Flutter's asset loader | ||
| /// to load localized JSON files. | ||
| /// | ||
| class RootBundleAssetLoader extends AssetLoader { | ||
| const RootBundleAssetLoader(); | ||
| // A custom asset loader that loads assets from the root bundle | ||
|
|
||
| String getLocalePath(String basePath, Locale locale) { | ||
| return '$basePath/${locale.toStringWithSeparator(separator: "-")}.json'; | ||
| const RootBundleAssetLoader(String path) : super(path: path); | ||
|
|
||
| /// Returns the path for the specified locale | ||
| /// | ||
| /// The [locale] parameter represents the desired locale. | ||
| /// The returned path is based on the [path] of the asset loader | ||
| /// and the [locale] with a separator ("-") between language and country. | ||
| String getLocalePath(Locale locale) { | ||
| return '$path/${locale.toStringWithSeparator(separator: "-")}.json'; | ||
| } | ||
|
|
||
| /// | ||
| /// Loads the localized JSON file for the given `locale`. | ||
| /// | ||
| /// Throws an `ArgumentError` if the `locale` is `null`. | ||
| /// | ||
| @override | ||
| Future<Map<String, dynamic>?> load(String path, Locale locale) async { | ||
| var localePath = getLocalePath(path, locale); | ||
| Future<Map<String, dynamic>> load({Locale? locale}) async { | ||
| if (locale == null) throw ArgumentError.notNull('locale'); | ||
| var localePath = getLocalePath(locale); | ||
| EasyLocalization.logger.debug('Load asset from $path'); | ||
|
|
||
| // Load the asset as a string and decode it as JSON | ||
| return json.decode(await rootBundle.loadString(localePath)); | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON parsing and file loading.
The implementation should handle potential errors when loading and parsing JSON files.
Consider adding try-catch blocks:
@override
Future<Map<String, dynamic>> load({Locale? locale}) async {
if (locale == null) throw ArgumentError.notNull('locale');
var localePath = getLocalePath(locale);
EasyLocalization.logger.debug('Load asset from $path');
- // Load the asset as a string and decode it as JSON
- return json.decode(await rootBundle.loadString(localePath));
+ try {
+ final jsonString = await rootBundle.loadString(localePath);
+ return json.decode(jsonString) as Map<String, dynamic>;
+ } on FlutterError catch (e) {
+ throw AssetLoadException('Failed to load $localePath: ${e.message}');
+ } on FormatException catch (e) {
+ throw AssetLoadException('Invalid JSON in $localePath: ${e.message}');
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| import 'package:easy_localization/src/asset_loader.dart'; | ||
|
|
||
| import 'package:flutter/widgets.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| // Replace with your package and file name | ||
|
|
||
| void main() { | ||
| TestWidgetsFlutterBinding.ensureInitialized(); | ||
| group('RootBundleAssetLoader', () { | ||
| const path = 'i18n'; // Replace with the actual asset path | ||
| const locale = Locale('en'); | ||
| late RootBundleAssetLoader assetLoader; | ||
|
|
||
| setUp(() async { | ||
| assetLoader = const RootBundleAssetLoader(path); | ||
| await assetLoader.load(locale: locale); | ||
| }); | ||
|
|
||
| test('getLocalePath returns the correct path', () async { | ||
| const expectedPath = '$path/en.json'; | ||
| var result = assetLoader.getLocalePath(locale); | ||
|
|
||
| expect(result, expectedPath); | ||
| }); | ||
|
|
||
| test('load throws an error when the locale is null', () { | ||
| assetLoader = const RootBundleAssetLoader(path); | ||
| expect(() => assetLoader.load(locale: null), throwsArgumentError); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with more test cases.
The test file needs additional test cases to cover error scenarios and actual asset loading.
Consider adding these test cases:
test('load throws AssetLoadException when file not found', () {
assetLoader = const RootBundleAssetLoader('invalid/path');
expect(
() => assetLoader.load(locale: locale),
throwsA(isA<AssetLoadException>()),
);
});
test('load throws AssetLoadException for invalid JSON', () {
// Mock rootBundle.loadString to return invalid JSON
expect(
() => assetLoader.load(locale: locale),
throwsA(isA<AssetLoadException>()),
);
});| // expect( | ||
| // EasyLocalization.of(_context)!.locale, const Locale('en', 'US')); | ||
| // }); | ||
| // }, | ||
| // ); | ||
|
|
||
| // testWidgets( | ||
| // '[EasyLocalization] device locale test', | ||
| // (WidgetTester tester) async { | ||
| // await tester.runAsync(() async { | ||
| // await tester.pumpWidget(EasyLocalization( | ||
| // // path: '../../i18n', | ||
| // // supportedLocales: const [Locale('en', 'US'), Locale('ar', 'DZ')], | ||
| // child: const MyApp(), // Locale('en', 'US'), Locale('ar','DZ') | ||
| // )); | ||
| // // await tester.idle(); | ||
| // // The async delegator load will require build on the next frame. Thus, pump | ||
| // await tester.pump(); | ||
|
|
||
| // expect(EasyLocalization.of(_context)!.deviceLocale.toString(), | ||
| // Platform.localeName); | ||
| // }); | ||
| // }, | ||
| // ); | ||
|
|
||
| // testWidgets( | ||
| // '[EasyLocalization] reset device locale test', | ||
| // (WidgetTester tester) async { | ||
| // await tester.runAsync(() async { | ||
| // await tester.pumpWidget(EasyLocalization( | ||
| // // path: '../../i18n', | ||
| // // supportedLocales: const [ | ||
| // // Locale('en', 'US'), | ||
| // // Locale('ar', 'DZ') | ||
| // // ], // Locale('en', 'US'), Locale('ar','DZ') | ||
| // startLocale: const Locale('ar', 'DZ'), | ||
| // child: const MyApp(), | ||
| // )); | ||
| // // await tester.idle(); | ||
| // // The async delegator load will require build on the next frame. Thus, pump | ||
| // await tester.pump(); | ||
|
|
||
| // expect( | ||
| // EasyLocalization.of(_context)!.locale, const Locale('ar', 'DZ')); | ||
| // // reset to device locale | ||
| // await _context.resetLocale(); | ||
| // await tester.pump(); | ||
| // expect( | ||
| // EasyLocalization.of(_context)!.locale, const Locale('en', 'US')); | ||
| // }); | ||
| // }, | ||
| // ); | ||
|
|
||
| // testWidgets( | ||
| // '[EasyLocalization] device locale test', | ||
| // (WidgetTester tester) async { | ||
| // await tester.runAsync(() async { | ||
| // await tester.pumpWidget(EasyLocalization( | ||
| // // path: '../../i18n', | ||
| // // supportedLocales: const [Locale('en', 'US'), Locale('ar', 'DZ')], | ||
| // child: const MyApp(), // Locale('en', 'US'), Locale('ar','DZ') | ||
| // )); | ||
| // await tester.idle(); | ||
| // // The async delegator load will require build on the next frame. Thus, pump | ||
| // await tester.pumpAndSettle(); | ||
|
|
||
| // expect(EasyLocalization.of(_context)!.deviceLocale.toString(), | ||
| // Platform.localeName); | ||
| // }); | ||
| // }, | ||
| // ); | ||
|
|
||
| // testWidgets( | ||
| // '[EasyLocalization] reset device locale test', | ||
| // (WidgetTester tester) async { | ||
| // await tester.runAsync(() async { | ||
| // await tester.pumpWidget(EasyLocalization( | ||
| // // path: '../../i18n', | ||
| // // supportedLocales: const [ | ||
| // // Locale('en', 'US'), | ||
| // // Locale('ar', 'DZ') | ||
| // // ], // Locale('en', 'US'), Locale('ar','DZ') | ||
| // startLocale: const Locale('ar', 'DZ'), | ||
| // child: const MyApp(), | ||
| // )); | ||
| // await tester.idle(); | ||
| // // The async delegator load will require build on the next frame. Thus, pump | ||
| // await tester.pumpAndSettle(); | ||
|
|
||
| // expect( | ||
| // EasyLocalization.of(_context)!.locale, const Locale('ar', 'DZ')); | ||
| // // reset to device locale | ||
| // await _context.resetLocale(); | ||
| // await tester.pumpAndSettle(); | ||
| // expect( | ||
| // EasyLocalization.of(_context)!.locale, const Locale('en', 'US')); | ||
| // }); | ||
| // }, | ||
| // ); | ||
| // }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up commented test groups.
Large sections of commented code make the file hard to maintain. Consider either removing them entirely or preserving essential tests with the new storage interface.
Hello everyone,
I have a suggestion for updating the next version of our project. I've written code to explain my idea and would greatly appreciate assistance in refining and implementing it.
Key Changes
Initialization Overhaul
supportedLocalesandpathfromEasyLocalizationwidget.ensureInitializedto loadAssetLoaderand storage early.AssetLoader Modernization
RootBundleAssetLoaderto require path during initialization.CachedAssetLoaderfor performance optimization.Storage Abstraction
SharedPreferencesusage withIEasyLocalizationStorage.SharedPreferencesStorageas a default implementation.Code Changes
1. Updated Initialization (
main.dart)void main() async { + WidgetsFlutterBinding.ensureInitialized(); + await EasyLocalization.ensureInitialized( + assetLoader: RootBundleAssetLoader('assets/translations'), + storage: SharedPreferencesStorage(), + ); runApp( EasyLocalization( - supportedLocales: const [Locale('en'), Locale('es')], - path: 'assets/translations', child: MyApp(), ), ); }2. New
SharedPreferencesStorageClass3. Optimized
CachedAssetLoaderMigration Guide (
MIGRATION.md)Remove Deprecated Parameters
Delete
supportedLocalesandpathfrom theEasyLocalizationwidget.Update AssetLoaders
Custom loaders must now accept a
pathand implementload().Adopt New Storage
Implement
IEasyLocalizationStoragefor locale persistence.Example:
SharedPreferencesStorage.Summary of Benefits
Task:
Summary by CodeRabbit
Release Notes for Easy Localization v4.0.0-dev.0
New Features
Breaking Changes
Improvements
Bug Fixes
Documentation