add easy_localization_storage, easy_localization_shared_preferences_storage and easy_localization_helper #719
Conversation
WalkthroughThe changes in this pull request primarily enhance localization functionality within a Flutter application. Key updates include the introduction of in-memory storage for localization data, replacing the previous reliance on 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: 10
🧹 Outside diff range and nitpick comments (12)
packages/easy_localization_storage/lib/src/easy_localization_storage.dart (2)
3-9: Add documentation to improve API clarity.Consider adding documentation to:
- Explain the purpose of the storage interface
- Document the version management purpose and valid values
- Document method parameters and return values
Example documentation:
/// Abstract interface for managing localization storage. /// Provides methods to persist and retrieve locale settings and version information. abstract class EasyLocalizationStorage { /// Retrieves the stored version number. /// Returns null if no version is stored. Future<int?> getVersion(); /// Stores the version number. /// Set [value] to null to clear the stored version. Future<void> setVersion({int? value}); /// Retrieves the stored locale. /// Returns null if no locale is stored. Future<Locale?> getLocale(); /// Stores the locale setting. /// Set [value] to null to clear the stored locale. Future<void> setLocale({Locale? value}); }
3-18: Consider adding error handling guidelines.As this is a core storage interface that will be implemented by different storage backends, consider:
- Documenting expected error scenarios and how implementations should handle them
- Adding guidance for error propagation (e.g., when storage is unavailable)
- Defining error types that implementations should throw
This will help ensure consistent behavior across different storage implementations.
packages/easy_localization_storage/lib/src/easy_localization_in_memory_storage.dart (2)
5-7: Enhance class documentation with usage examples and memory implications.While the documentation indicates this is for testing, it would be helpful to:
- Add example usage code
- Document that data is lost when the app restarts
- Mention any performance characteristics or memory implications
Add the following documentation:
/// An in-memory implementation of [EasyLocalizationStorage]. /// Useful for testing. +/// +/// Note: Data is stored in memory and will be lost when the app restarts. +/// +/// Example usage: +/// ```dart +/// final storage = EasyLocalizationInMemoryStorage(); +/// await EasyLocalization.ensureInitialized( +/// storage: storage, +/// // ... other parameters +/// ); +/// ```
8-9: Document the significance of initial version value.The initial version value of 1 should be documented to explain its significance and why it's not 0 or null by default.
Add documentation above the version declaration:
+ /// The storage version, initialized to 1. + /// This helps track and manage storage format changes. int? _version = 1;packages/easy_localization_helper/lib/easy_localization_helper.dart (1)
5-11: Documentation and edge cases need attention.While the implementation is straightforward, consider these improvements:
- Enhance documentation to include:
- Example usage
- Parameter description
- Return value format
- Add validation for the separator parameter
- Handle potential edge cases in the Locale's toString() output
Consider updating the documentation and implementation:
/// [Easy Localization] locale helper extension LocaleToStringHelper on Locale { - /// Convert [locale] to String with custom separator + /// Converts a Locale to a string representation using a custom separator. + /// + /// Example: + /// ```dart + /// final locale = Locale('en', 'US'); + /// print(locale.toStringWithSeparator(separator: '-')); // outputs: en-US + /// ``` + /// + /// Parameters: + /// separator: The string to use as separator between locale components. + /// Defaults to '_'. + /// + /// Returns: + /// A string representation of the locale with the specified separator. String toStringWithSeparator({String separator = '_'}) { + if (separator.isEmpty) { + throw ArgumentError('Separator cannot be empty'); + } return toString().split('_').join(separator); } }packages/easy_localization_shared_preferences_storage/lib/src/easy_localization_shared_preferences_storage.dart (2)
8-16: Add documentation for the constructor parameters and fields.Consider adding dartdoc comments to explain:
- The purpose of
versionparameter- The format and usage of
storagePrefix- The significance of the generated keys
+ /// Storage implementation using SharedPreferences. + /// + /// @param store The SharedPreferences instance to use for storage + /// @param version The storage version, used for migration (defaults to 1) + /// @param storagePrefix The prefix for storage keys (defaults to 'easy_localization.locale') class EasyLocalizationSharedPreferencesStorage extends EasyLocalizationStorage {
19-19: Consider implementing the migration method.The TODO comment indicates a need for version-based migration. This is important for handling storage format changes across app updates.
Would you like me to help design and implement the migration mechanism?
example/pubspec.yaml (1)
28-29: Add version constraints for the new dependency.While using relative paths is fine for local development, it's recommended to specify version constraints for better dependency management.
easy_localization_storage: - path: ../packages/easy_localization_storage + path: ../packages/easy_localization_storage + version: ^1.0.0 # Replace with appropriate versiontest/easy_localization_context_test.dart (1)
Line range hint
183-248: Remove duplicate test casesThe following test cases are duplicated:
- "[EasyLocalization] device locale test"
- "[EasyLocalization] reset device locale test"
The only difference is the use of
pumpAndSettle()instead ofpump(). Consider:
- Removing the duplicates
- If different timing behavior needs testing, rename the tests to clarify the distinction
test/easy_localization_test.dart (1)
Line range hint
222-254: Fix typo in test name and LGTM for locale matching tests.The new test cases thoroughly cover locale matching scenarios, addressing issue #674. However, there's a minor typo in the test name.
- test('select best lenguage match if no perfect match exists', () { + test('select best language match if no perfect match exists', () {test/easy_localization_widget_test.dart (1)
426-430: Consider removing commented-out code.The SharedPreferences mock setup is commented out but no longer needed since we've moved to in-memory storage.
- // setUp(() { - // SharedPreferences.setMockInitialValues({ - // 'locale': '', - // }); - // });lib/src/easy_localization_controller.dart (1)
177-181: Consider renaming 'useOnlyLangCode' to 'ignoreCountryCode' for clarityThe variable
useOnlyLangCodecontrols whether thecountryCodeis used when constructing thedesiredLocale. Since thescriptCodeis still included, renaming it toignoreCountryCodemight better reflect its purpose and improve code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (42)
- example/lib/main.dart (8 hunks)
- example/macos/Flutter/GeneratedPluginRegistrant.swift (0 hunks)
- example/pubspec.yaml (1 hunks)
- lib/src/asset_loader.dart (1 hunks)
- lib/src/easy_localization_app.dart (2 hunks)
- lib/src/easy_localization_controller.dart (3 hunks)
- lib/src/utils.dart (0 hunks)
- packages/easy_localization_helper/.gitignore (1 hunks)
- packages/easy_localization_helper/.metadata (1 hunks)
- packages/easy_localization_helper/CHANGELOG.md (1 hunks)
- packages/easy_localization_helper/LICENSE (1 hunks)
- packages/easy_localization_helper/README.md (1 hunks)
- packages/easy_localization_helper/analysis_options.yaml (1 hunks)
- packages/easy_localization_helper/lib/easy_localization_helper.dart (1 hunks)
- packages/easy_localization_helper/pubspec.yaml (1 hunks)
- packages/easy_localization_helper/test/easy_localization_helper_test.dart (1 hunks)
- packages/easy_localization_shared_preferences_storage/.gitignore (1 hunks)
- packages/easy_localization_shared_preferences_storage/.metadata (1 hunks)
- packages/easy_localization_shared_preferences_storage/CHANGELOG.md (1 hunks)
- packages/easy_localization_shared_preferences_storage/LICENSE (1 hunks)
- packages/easy_localization_shared_preferences_storage/README.md (1 hunks)
- packages/easy_localization_shared_preferences_storage/analysis_options.yaml (1 hunks)
- packages/easy_localization_shared_preferences_storage/lib/easy_localization_shared_preferences_storage.dart (1 hunks)
- packages/easy_localization_shared_preferences_storage/lib/src/easy_localization_shared_preferences_storage.dart (1 hunks)
- packages/easy_localization_shared_preferences_storage/pubspec.yaml (1 hunks)
- packages/easy_localization_shared_preferences_storage/test/easy_localization_shared_preferences_storage_test.dart (1 hunks)
- packages/easy_localization_storage/.gitignore (1 hunks)
- packages/easy_localization_storage/.metadata (1 hunks)
- packages/easy_localization_storage/CHANGELOG.md (1 hunks)
- packages/easy_localization_storage/LICENSE (1 hunks)
- packages/easy_localization_storage/README.md (1 hunks)
- packages/easy_localization_storage/analysis_options.yaml (1 hunks)
- packages/easy_localization_storage/lib/easy_localization_storage.dart (1 hunks)
- packages/easy_localization_storage/lib/src/easy_localization_in_memory_storage.dart (1 hunks)
- packages/easy_localization_storage/lib/src/easy_localization_storage.dart (1 hunks)
- packages/easy_localization_storage/pubspec.yaml (1 hunks)
- packages/easy_localization_storage/test/easy_localization_storage_test.dart (1 hunks)
- pubspec.yaml (2 hunks)
- test/easy_localization_context_test.dart (3 hunks)
- test/easy_localization_test.dart (9 hunks)
- test/easy_localization_utils_test.dart (1 hunks)
- test/easy_localization_widget_test.dart (6 hunks)
💤 Files with no reviewable changes (2)
- example/macos/Flutter/GeneratedPluginRegistrant.swift
- lib/src/utils.dart
✅ Files skipped from review due to trivial changes (25)
- lib/src/asset_loader.dart
- packages/easy_localization_helper/.gitignore
- packages/easy_localization_helper/.metadata
- packages/easy_localization_helper/CHANGELOG.md
- packages/easy_localization_helper/LICENSE
- packages/easy_localization_helper/analysis_options.yaml
- packages/easy_localization_helper/pubspec.yaml
- packages/easy_localization_helper/test/easy_localization_helper_test.dart
- packages/easy_localization_shared_preferences_storage/.gitignore
- packages/easy_localization_shared_preferences_storage/.metadata
- packages/easy_localization_shared_preferences_storage/CHANGELOG.md
- packages/easy_localization_shared_preferences_storage/LICENSE
- packages/easy_localization_shared_preferences_storage/analysis_options.yaml
- packages/easy_localization_shared_preferences_storage/lib/easy_localization_shared_preferences_storage.dart
- packages/easy_localization_shared_preferences_storage/pubspec.yaml
- packages/easy_localization_shared_preferences_storage/test/easy_localization_shared_preferences_storage_test.dart
- packages/easy_localization_storage/.gitignore
- packages/easy_localization_storage/.metadata
- packages/easy_localization_storage/CHANGELOG.md
- packages/easy_localization_storage/LICENSE
- packages/easy_localization_storage/analysis_options.yaml
- packages/easy_localization_storage/lib/easy_localization_storage.dart
- packages/easy_localization_storage/pubspec.yaml
- packages/easy_localization_storage/test/easy_localization_storage_test.dart
- test/easy_localization_utils_test.dart
🧰 Additional context used
🪛 LanguageTool
packages/easy_localization_helper/README.md
[uncategorized] ~29-~29: You might be missing the article “the” here.
Context: ...r package users. Add longer examples to/examplefolder. ```dart const like = ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
packages/easy_localization_shared_preferences_storage/README.md
[style] ~14-~14: Consider using the synonym “brief” (= concise, using a few words, not lasting long) to strengthen your wording.
Context: .../to/develop-packages). --> TODO: Put a short description of the package here that he...(QUICK_BRIEF)
[uncategorized] ~29-~29: You might be missing the article “the” here.
Context: ...r package users. Add longer examples to/examplefolder. ```dart const like = ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
packages/easy_localization_storage/README.md
[style] ~14-~14: Consider using the synonym “brief” (= concise, using a few words, not lasting long) to strengthen your wording.
Context: .../to/develop-packages). --> TODO: Put a short description of the package here that he...(QUICK_BRIEF)
[uncategorized] ~29-~29: You might be missing the article “the” here.
Context: ...r package users. Add longer examples to/examplefolder. ```dart const like = ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (24)
packages/easy_localization_storage/lib/src/easy_localization_storage.dart (1)
1-1: LGTM!The import is correctly specified and necessary for the Locale type.
packages/easy_localization_storage/lib/src/easy_localization_in_memory_storage.dart (1)
11-20: LGTM! Well-implemented version management.The version management implementation includes:
- Proper error handling for negative values
- Clear error messages
- Correct async/Future implementation
pubspec.yaml (2)
20-23: Consider version constraints for local packages.While using path dependencies is fine for development, consider adding version constraints for these packages to ensure compatibility when published:
dependencies: easy_localization_storage: ^1.0.0 easy_localization_helper: ^1.0.0This change aligns with the architectural improvements mentioned in the PR objectives, separating storage and helper functionalities into dedicated packages.
✅ Verification successful
Version constraints should be added based on existing package versions
Since both packages are at version
0.0.1, I recommend using version constraints that reflect their early development stage:dependencies: easy_localization_storage: ^0.0.1 easy_localization_helper: ^0.0.1This ensures compatibility with the current package versions while allowing for patch updates. As these packages mature, you can update the constraints accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the new packages have their own pubspec.yaml with version info echo "Checking easy_localization_storage package:" cat packages/easy_localization_storage/pubspec.yaml 2>/dev/null | grep "version:" echo -e "\nChecking easy_localization_helper package:" cat packages/easy_localization_helper/pubspec.yaml 2>/dev/null | grep "version:"Length of output: 366
6-6: Verify ifpublish_to: noneis intended for development only.The uncommented
publish_to: noneprevents publishing to pub.dev. Ensure this is intended only for development and should be commented out or removed before the final release.✅ Verification successful
publish_to: noneis correctly configured across all packagesThe
publish_to: nonesetting is consistently applied across all packages in the monorepo structure (main package, storage, helper, shared preferences storage, and example). This is the intended configuration for a monorepo where packages are meant to be used together and not published individually to pub.dev.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this is consistent with other packages in the repository fd -e yaml pubspec.yaml -x grep -l "publish_to:"Length of output: 254
Script:
#!/bin/bash # Check the actual publish_to configuration in all pubspec.yaml files echo "=== Main package ===" grep -A 1 "publish_to:" ./pubspec.yaml || true echo -e "\n=== Storage package ===" grep -A 1 "publish_to:" ./packages/easy_localization_storage/pubspec.yaml || true echo -e "\n=== Helper package ===" grep -A 1 "publish_to:" ./packages/easy_localization_helper/pubspec.yaml || true echo -e "\n=== Shared Preferences Storage package ===" grep -A 1 "publish_to:" ./packages/easy_localization_shared_preferences_storage/pubspec.yaml || true echo -e "\n=== Example package ===" grep -A 1 "publish_to:" ./example/pubspec.yaml || trueLength of output: 828
packages/easy_localization_helper/lib/easy_localization_helper.dart (2)
1-4: LGTM! Clean library declaration with appropriate imports.
13-36:⚠️ Potential issueAdd input validation and improve error handling.
The current implementation makes assumptions about the input format without proper validation, which could lead to runtime errors.
Consider implementing these improvements:
/// [Easy Localization] string locale helper extension StringToLocaleHelper on String { - /// Convert string to [Locale] object + /// Converts a string to a Locale object based on a specified separator. + /// + /// Format: <languageCode>[separator][scriptCode/countryCode][separator][countryCode] + /// + /// Examples: + /// ```dart + /// 'en_US'.toLocale() // Locale('en', 'US') + /// 'zh_Hant_TW'.toLocale() // Locale.fromSubtags(languageCode: 'zh', scriptCode: 'Hant', countryCode: 'TW') + /// 'sr_Cyrl'.toLocale() // Locale.fromSubtags(languageCode: 'sr', scriptCode: 'Cyrl') + /// ``` + /// + /// Throws: + /// FormatException if the string format is invalid Locale toLocale({String separator = '_'}) { + if (isEmpty) { + throw FormatException('Locale string cannot be empty'); + } + if (separator.isEmpty) { + throw ArgumentError('Separator cannot be empty'); + } + final localeList = split(separator); + + // Validate language code + if (localeList.first.isEmpty || !RegExp(r'^[a-zA-Z]{2,3}$').hasMatch(localeList.first)) { + throw FormatException('Invalid language code: ${localeList.first}'); + } + switch (localeList.length) { case 2: - return localeList.last.length == 4 // scriptCode length is 4 + final secondPart = localeList.last; + if (secondPart.isEmpty) { + throw FormatException('Second part cannot be empty'); + } + + return secondPart.length == 4 // scriptCode length is 4 ? Locale.fromSubtags( languageCode: localeList.first, - scriptCode: localeList.last, + scriptCode: secondPart, ) - : Locale(localeList.first, localeList.last); + : Locale(localeList.first, secondPart); case 3: + if (localeList[1].isEmpty || localeList.last.isEmpty) { + throw FormatException('Script code and country code cannot be empty'); + } + if (localeList[1].length != 4) { + throw FormatException('Invalid script code: ${localeList[1]}'); + } return Locale.fromSubtags( languageCode: localeList.first, scriptCode: localeList[1], countryCode: localeList.last, ); default: + if (localeList.length > 3) { + throw FormatException('Too many components in locale string'); + } return Locale(localeList.first); } } }Let's verify the usage patterns in the codebase:
packages/easy_localization_shared_preferences_storage/lib/src/easy_localization_shared_preferences_storage.dart (2)
1-7: LGTM! Clean imports and proper class hierarchy.The imports are well-organized and the class correctly extends
EasyLocalizationStorage.
7-7: Verify storage interface implementation completeness.Let's ensure this implementation satisfies all requirements of the storage interface.
✅ Verification successful
Storage interface implementation is complete and correct
The implementation properly implements all required methods from the
EasyLocalizationStorageinterface:
Future<int?> getVersion()Future<void> setVersion({int? value})Future<Locale?> getLocale()Future<void> setLocale({Locale? value})Each method has appropriate error handling and correctly uses the underlying SharedPreferences store for persistence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find and analyze the storage interface definition ast-grep --pattern 'abstract class EasyLocalizationStorage { $$$ }' # Check for any additional interface methods that might need implementation rg -A 5 "abstract.*Future" packages/easy_localization_storageLength of output: 855
Script:
#!/bin/bash # Check the implementation of the required methods in SharedPreferences storage ast-grep --pattern 'class EasyLocalizationSharedPreferencesStorage extends EasyLocalizationStorage { $$$ }' # Also check for method implementations specifically ast-grep --pattern 'Future<int?> getVersion() { $$$ }' ast-grep --pattern 'Future<void> setVersion({int? value}) { $$$ }' ast-grep --pattern 'Future<Locale?> getLocale() { $$$ }' ast-grep --pattern 'Future<void> setLocale({Locale? value}) { $$$ }'Length of output: 8893
example/pubspec.yaml (1)
28-29: Verify the dependency structure.Let's verify if the new packages are properly structured and if there are any circular dependencies.
✅ Verification successful
Let me check the actual content of these pubspec.yaml files to verify the dependency structure and potential circular dependencies.
Dependency structure is properly organized
The package structure is well-organized with no circular dependencies:
easy_localization_storageandeasy_localization_helperare independent base packages with only Flutter SDK dependencieseasy_localization_shared_preferences_storageproperly depends on both base packages and shared_preferences- All packages have consistent SDK constraints and proper path references
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the structure of the new packages and their dependencies # Test 1: Verify the existence of the new packages echo "Checking package directories..." fd -t d "easy_localization_(storage|helper)" packages/ # Test 2: Check for pubspec.yaml files in the new packages echo -e "\nChecking pubspec.yaml files..." fd -t f "pubspec.yaml" packages/easy_localization_storage packages/easy_localization_helper # Test 3: Look for potential circular dependencies echo -e "\nChecking for circular dependencies..." rg -l "easy_localization(_storage|_helper)?" packages/easy_localization_*/pubspec.yamlLength of output: 926
Script:
#!/bin/bash # Check the content of pubspec.yaml files to analyze dependencies echo "Content of easy_localization_storage/pubspec.yaml:" cat packages/easy_localization_storage/pubspec.yaml echo -e "\nContent of easy_localization_helper/pubspec.yaml:" cat packages/easy_localization_helper/pubspec.yaml echo -e "\nContent of easy_localization_shared_preferences_storage/pubspec.yaml:" cat packages/easy_localization_shared_preferences_storage/pubspec.yamlLength of output: 5432
example/lib/main.dart (4)
2-2: Verify storage persistence requirements.The switch to
EasyLocalizationInMemoryStoragemeans locale settings won't persist across app restarts. Consider if this meets your application's requirements.If persistence is needed, consider using
EasyLocalizationSharedPreferencesStorageinstead:- await EasyLocalization.ensureInitialized(EasyLocalizationInMemoryStorage()); + await EasyLocalization.ensureInitialized(EasyLocalizationSharedPreferencesStorage());Also applies to: 11-12
Line range hint
14-43: LGTM! Performance improvements with const.Good use of const for supportedLocales and child property, which helps with widget rebuilding optimization.
Line range hint
44-58: LGTM! Proper usage of const constructor.Good implementation of const constructor for the stateless widget, which helps with widget rebuilding optimization.
131-134: LGTM! Proper usage of const for static text widgets.Good use of const for Text widgets that have static content or use tr()/plural() for translation.
Also applies to: 139-139, 155-155, 163-163
test/easy_localization_context_test.dart (2)
4-4: LGTM: Import added for new storage packageThe import aligns with the PR's objective of introducing new storage components.
54-54: Verify initialization behavior with in-memory storageThe switch to explicit in-memory storage is a breaking change. Ensure that:
- Test isolation is maintained between test cases
- Storage state is properly reset between tests
✅ Verification successful
Let me search for test cases to verify test isolation and state reset.
Let me check how the in-memory storage is implemented to verify its state management.
Let me try a different search pattern to locate the storage implementation.
Storage implementation is safe for test isolation
The switch to
EasyLocalizationInMemoryStorageis actually beneficial for test isolation:
- Each test group properly initializes its own storage instance in
setUpAll- The storage implementation is simple with in-memory variables that don't persist between tests
- Storage state is scoped to individual test groups through separate initialization
The implementation in
easy_localization_in_memory_storage.dartshows it's just maintaining state in memory variables (_versionand_locale) which are automatically cleaned up between test runs, ensuring proper test isolation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any test setup/teardown methods that might need updating rg -A 5 "setUp|tearDown" test/Length of output: 5486
Script:
#!/bin/bash # Search for test cases in the context file to verify isolation rg -B 2 -A 10 "testWidgets|test\(" test/easy_localization_context_test.dartLength of output: 3877
Script:
#!/bin/bash # Search for the implementation of EasyLocalizationInMemoryStorage rg -A 10 "class EasyLocalizationInMemoryStorage" lib/Length of output: 55
Script:
#!/bin/bash # Search for in-memory storage implementation with a broader pattern rg -A 10 "InMemoryStorage" .Length of output: 9003
lib/src/easy_localization_app.dart (2)
5-5: LGTM: Import aligns with new storage functionality.The addition of the easy_localization_storage import is appropriate for the new storage management functionality being introduced.
163-164:⚠️ Potential issueBreaking change: Update documentation and consider null safety.
This modification to
ensureInitializedintroduces a breaking change that requires attention:
- The method now requires a storage parameter, which will affect all existing implementations
- The documentation above the method (lines 159-162) needs to be updated to reflect the new parameter
- Consider adding null safety checks for the storage parameter
Let's verify the impact of this breaking change:
Consider the following recommendations:
- Update the documentation to include:
- Description of the storage parameter
- Migration guide for existing implementations
- Example usage with the new storage parameter
- Consider providing a default storage implementation to maintain backward compatibility
✅ Verification successful
Let me verify the documentation and null safety aspects with additional scripts:
Let me check one more aspect - the implementation of initEasyLocation method since it's being called:
Breaking change is properly documented and handled
The breaking change is already well-documented and implemented correctly:
- The CHANGELOG.md explicitly marks breaking changes
- The documentation above the method clearly states its initialization requirement
- The implementation in EasyLocalizationController properly handles the storage parameter:
- Storage is set and validated
- Error message exists for uninitialized state
- Clear examples exist in README.md showing usage
No additional action is needed as the changes follow best practices for breaking changes with proper documentation, error handling, and migration examples.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing calls to ensureInitialized to assess migration needs rg "EasyLocalization\.ensureInitialized\(\)" -A 2Length of output: 885
Script:
#!/bin/bash # Check the documentation of ensureInitialized method rg -B 5 "static Future<void> ensureInitialized" lib/ # Check if there are any null checks in the implementation ast-grep --pattern 'static Future<void> ensureInitialized(EasyLocalizationStorage storage) { $$$ }' # Check the implementation of initEasyLocation ast-grep --pattern 'static Future<void> initEasyLocation(EasyLocalizationStorage storage) { $$$ }'Length of output: 717
Script:
#!/bin/bash # Search for initEasyLocation implementation and its usage rg "initEasyLocation" -A 5 lib/Length of output: 1443
test/easy_localization_test.dart (4)
6-7: LGTM: New package imports align with PR objectives.The addition of helper and storage packages aligns with the PR's goal of enhancing localization storage capabilities.
136-137: LGTM: Clean transition to in-memory storage.The initialization properly uses the new storage mechanism while maintaining the same functionality.
156-157: LGTM: Good coverage of fallback locale behavior.The test properly verifies the fallback behavior when a saved locale is no longer supported.
Line range hint
554-678: LGTM: Comprehensive testing of empty resource handling.The test cases thoroughly verify:
- Empty resource fallback behavior
- Proper error reporting
- Edge cases in plural form handling
test/easy_localization_widget_test.dart (2)
6-6: LGTM: Import added for the new storage package.The import of
easy_localization_storageis correctly placed and aligns with the PR objectives of introducing new storage components.
78-78: LGTM: Initialization updated to use in-memory storage.The
EasyLocalization.ensureInitialized()call has been correctly updated to use the new storage mechanism.lib/src/easy_localization_controller.dart (1)
240-241:⚠️ Potential issueFix variable interpolation in the logging statement
The logging statement escapes the dollar signs, preventing variable interpolation. Remove the backslashes to correctly interpolate the variables.
Apply this diff to fix the interpolation:
- 'Reset locale to \$locale while the platform locale is \$_deviceLocale and the fallback locale is \$_fallbackLocale'); + 'Reset locale to $locale while the platform locale is $_deviceLocale and the fallback locale is $_fallbackLocale');Likely invalid or redundant comment.
| extension XEasyLocalizationStorage on EasyLocalizationStorage { | ||
| Future reset() { | ||
| return Future.value([ | ||
| setVersion(value: null), | ||
| setLocale(value: null), | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix concurrent operations handling and naming convention.
The current implementation has two issues:
- The reset method doesn't wait for the futures to complete
- The 'X' prefix in the extension name doesn't follow Dart naming conventions
Apply this diff to fix the issues:
-extension XEasyLocalizationStorage on EasyLocalizationStorage {
- Future reset() {
- return Future.value([
- setVersion(value: null),
- setLocale(value: null),
- ]);
+extension EasyLocalizationStorageExtension on EasyLocalizationStorage {
+ /// Resets both version and locale to null.
+ Future<void> reset() async {
+ await Future.wait([
+ setVersion(value: null),
+ setLocale(value: null),
+ ]);
}
}This change:
- Uses
Future.waitto properly await both operations - Follows Dart naming conventions for extensions
- Adds return type void for clarity
- Adds documentation
📝 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.
| extension XEasyLocalizationStorage on EasyLocalizationStorage { | |
| Future reset() { | |
| return Future.value([ | |
| setVersion(value: null), | |
| setLocale(value: null), | |
| ]); | |
| } | |
| } | |
| extension EasyLocalizationStorageExtension on EasyLocalizationStorage { | |
| /// Resets both version and locale to null. | |
| Future<void> reset() async { | |
| await Future.wait([ | |
| setVersion(value: null), | |
| setLocale(value: null), | |
| ]); | |
| } | |
| } |
| @override | ||
| Future<Locale?> getLocale() async => _locale; | ||
|
|
||
| @override | ||
| Future<void> setLocale({Locale? value}) async => _locale = value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding locale validation.
The setLocale method accepts any locale without validation. Consider adding checks for:
- Null locale handling strategy
- Invalid locale formats
- Unsupported locales
Here's a suggested implementation with validation:
@override
Future<Locale?> getLocale() async => _locale;
@override
- Future<void> setLocale({Locale? value}) async => _locale = value;
+ Future<void> setLocale({Locale? value}) async {
+ if (value != null) {
+ // Validate locale format
+ if (value.languageCode.isEmpty) {
+ throw ArgumentError('Language code cannot be empty');
+ }
+
+ // Optional: Add validation for supported locales
+ // if (!supportedLocales.contains(value)) {
+ // throw ArgumentError('Unsupported locale: $value');
+ // }
+ }
+ _locale = value;
+ }Committable suggestion was skipped due to low confidence.
| import 'dart:ui'; | ||
|
|
||
| import 'easy_localization_storage.dart'; | ||
|
|
||
| /// An in-memory implementation of [EasyLocalizationStorage]. | ||
| /// Useful for testing. | ||
| class EasyLocalizationInMemoryStorage implements EasyLocalizationStorage { | ||
| int? _version = 1; | ||
| Locale? _locale; | ||
|
|
||
| @override | ||
| Future<int?> getVersion() async => _version; | ||
|
|
||
| @override | ||
| Future<void> setVersion({int? value}) async { | ||
| if (value != null && value < 0) { | ||
| throw ArgumentError('Value must be null or non-negative'); | ||
| } | ||
| _version = value; | ||
| } | ||
|
|
||
| @override | ||
| Future<Locale?> getLocale() async => _locale; | ||
|
|
||
| @override | ||
| Future<void> setLocale({Locale? value}) async => _locale = value; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing unit tests for EasyLocalizationInMemoryStorage
While the implementation is used in integration tests across the codebase (easy_localization_test.dart, easy_localization_widget_test.dart, and easy_localization_context_test.dart), there are no dedicated unit tests for the storage implementation itself. The storage test file exists but is commented out and contains unrelated test code.
packages/easy_localization_storage/test/easy_localization_storage_test.dart: Add unit tests to verify:- Version getter/setter with valid and invalid values
- Locale getter/setter functionality
- Error handling for negative version values
🔗 Analysis chain
Verify test coverage for the storage implementation.
Since this is a test-focused implementation, we should ensure proper test coverage exists.
Let me gather more information about the test coverage by examining the actual test implementations.
Let me check if there are specific tests for the storage implementation itself, as the current results only show integration tests.
Let me check the content of the storage test file we found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files covering this implementation
echo "Searching for test files..."
fd -e dart test easy_localization_in_memory_storage
# Search for usage in test files
echo "Searching for usage in tests..."
rg "EasyLocalizationInMemoryStorage" -t dart
Length of output: 1536
Script:
#!/bin/bash
# Check the test implementations that use EasyLocalizationInMemoryStorage
echo "Checking test/easy_localization_test.dart..."
rg -A 10 "EasyLocalizationInMemoryStorage" test/easy_localization_test.dart
echo "\nChecking test/easy_localization_widget_test.dart..."
rg -A 10 "EasyLocalizationInMemoryStorage" test/easy_localization_widget_test.dart
echo "\nChecking test/easy_localization_context_test.dart..."
rg -A 10 "EasyLocalizationInMemoryStorage" test/easy_localization_context_test.dart
Length of output: 4125
Script:
#!/bin/bash
# Look for storage-specific tests
echo "Checking for storage-specific tests..."
fd -e dart test packages/easy_localization_storage
# Check the implementation details of storage tests if they exist
echo "Checking storage test implementations..."
rg -t dart "describe|test.*EasyLocalizationInMemoryStorage" packages/easy_localization_storage/test
Length of output: 408
Script:
#!/bin/bash
# Check the content of the storage test file
echo "Checking storage test file content..."
cat packages/easy_localization_storage/test/easy_localization_storage_test.dart
Length of output: 534
| @override | ||
| Future<int?> getVersion() async { | ||
| int? value; | ||
| try { | ||
| value = _store.getInt(versionKey); | ||
| } catch (_) {} | ||
| return value; | ||
| } | ||
|
|
||
| @override | ||
| Future<void> setVersion({int? value}) async { | ||
| try { | ||
| if (value == null) { | ||
| _store.remove(versionKey); | ||
| } else { | ||
| _store.setInt(versionKey, value); | ||
| } | ||
| } catch (_) {} | ||
| } |
There was a problem hiding this comment.
Improve error handling in version management methods.
The empty catch blocks silently swallow all errors, which could make debugging difficult. Consider:
- Logging errors for debugging
- Handling specific exceptions
- Potentially propagating critical errors
Future<int?> getVersion() async {
int? value;
try {
value = _store.getInt(versionKey);
- } catch (_) {}
+ } catch (e) {
+ debugPrint('Failed to get version: $e');
+ }
return value;
}
Future<void> setVersion({int? value}) async {
try {
if (value == null) {
_store.remove(versionKey);
} else {
_store.setInt(versionKey, value);
}
- } catch (_) {}
+ } catch (e) {
+ debugPrint('Failed to set version: $e');
+ rethrow; // Consider rethrowing as this is a critical operation
+ }
}📝 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.
| @override | |
| Future<int?> getVersion() async { | |
| int? value; | |
| try { | |
| value = _store.getInt(versionKey); | |
| } catch (_) {} | |
| return value; | |
| } | |
| @override | |
| Future<void> setVersion({int? value}) async { | |
| try { | |
| if (value == null) { | |
| _store.remove(versionKey); | |
| } else { | |
| _store.setInt(versionKey, value); | |
| } | |
| } catch (_) {} | |
| } | |
| @override | |
| Future<int?> getVersion() async { | |
| int? value; | |
| try { | |
| value = _store.getInt(versionKey); | |
| } catch (e) { | |
| debugPrint('Failed to get version: $e'); | |
| } | |
| return value; | |
| } | |
| @override | |
| Future<void> setVersion({int? value}) async { | |
| try { | |
| if (value == null) { | |
| _store.remove(versionKey); | |
| } else { | |
| _store.setInt(versionKey, value); | |
| } | |
| } catch (e) { | |
| debugPrint('Failed to set version: $e'); | |
| rethrow; // Consider rethrowing as this is a critical operation | |
| } | |
| } |
| @override | ||
| Future<Locale?> getLocale() async { | ||
| Locale? value; | ||
| try { | ||
| final raw = _store.getString(valueKey); | ||
| if (raw == null) { | ||
| return null; | ||
| } | ||
| return raw.toLocale(); | ||
| } catch (_) {} | ||
| return value; | ||
| } | ||
|
|
||
| @override | ||
| Future<void> setLocale({Locale? value}) async { | ||
| try { | ||
| if (value == null) { | ||
| _store.remove(valueKey); | ||
| } else { | ||
| _store.setString(valueKey, value.toString()); | ||
| } | ||
| } catch (_) {} | ||
| } |
There was a problem hiding this comment.
Fix error handling and simplify locale management methods.
Several issues to address:
- Empty catch blocks hide errors
- No validation of locale string format
- Redundant value variable in getLocale()
Future<Locale?> getLocale() async {
- Locale? value;
try {
final raw = _store.getString(valueKey);
if (raw == null) {
return null;
}
+ // Validate locale format before parsing
+ if (!raw.contains('_') && !raw.contains('-')) {
+ debugPrint('Invalid locale format: $raw');
+ return null;
+ }
return raw.toLocale();
- } catch (_) {}
- return value;
+ } catch (e) {
+ debugPrint('Failed to get locale: $e');
+ return null;
+ }
}
Future<void> setLocale({Locale? value}) async {
try {
if (value == null) {
_store.remove(valueKey);
} else {
_store.setString(valueKey, value.toString());
}
- } catch (_) {}
+ } catch (e) {
+ debugPrint('Failed to set locale: $e');
+ rethrow; // Consider rethrowing as this is a critical operation
+ }
}📝 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.
| @override | |
| Future<Locale?> getLocale() async { | |
| Locale? value; | |
| try { | |
| final raw = _store.getString(valueKey); | |
| if (raw == null) { | |
| return null; | |
| } | |
| return raw.toLocale(); | |
| } catch (_) {} | |
| return value; | |
| } | |
| @override | |
| Future<void> setLocale({Locale? value}) async { | |
| try { | |
| if (value == null) { | |
| _store.remove(valueKey); | |
| } else { | |
| _store.setString(valueKey, value.toString()); | |
| } | |
| } catch (_) {} | |
| } | |
| @override | |
| Future<Locale?> getLocale() async { | |
| try { | |
| final raw = _store.getString(valueKey); | |
| if (raw == null) { | |
| return null; | |
| } | |
| // Validate locale format before parsing | |
| if (!raw.contains('_') && !raw.contains('-')) { | |
| debugPrint('Invalid locale format: $raw'); | |
| return null; | |
| } | |
| return raw.toLocale(); | |
| } catch (e) { | |
| debugPrint('Failed to get locale: $e'); | |
| return null; | |
| } | |
| } | |
| @override | |
| Future<void> setLocale({Locale? value}) async { | |
| try { | |
| if (value == null) { | |
| _store.remove(valueKey); | |
| } else { | |
| _store.setString(valueKey, value.toString()); | |
| } | |
| } catch (e) { | |
| debugPrint('Failed to set locale: $e'); | |
| rethrow; // Consider rethrowing as this is a critical operation | |
| } | |
| } |
| easy_localization_storage: | ||
| path: ../packages/easy_localization_storage |
There was a problem hiding this comment.
Missing dependency: easy_localization_helper
According to the PR objectives and AI summary, this PR introduces both easy_localization_storage and easy_localization_helper packages. However, the easy_localization_helper dependency is missing from the pubspec.yaml.
Add the missing dependency:
easy_localization_storage:
path: ../packages/easy_localization_storage
+easy_localization_helper:
+ path: ../packages/easy_localization_helper📝 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.
| easy_localization_storage: | |
| path: ../packages/easy_localization_storage | |
| easy_localization_storage: | |
| path: ../packages/easy_localization_storage | |
| easy_localization_helper: | |
| path: ../packages/easy_localization_helper |
| children: <Widget>[ | ||
| FaIcon(FontAwesomeIcons.male), | ||
| const FaIcon(FontAwesomeIcons.male), | ||
| Switch(value: _gender, onChanged: switchGender), | ||
| FaIcon(FontAwesomeIcons.female), | ||
| const FaIcon(FontAwesomeIcons.female), | ||
| ], |
There was a problem hiding this comment.
Enhance gender switch accessibility.
The gender switch lacks proper accessibility labels which could make it difficult for screen readers.
Add semantic labels:
- const FaIcon(FontAwesomeIcons.male),
- Switch(value: _gender, onChanged: switchGender),
- const FaIcon(FontAwesomeIcons.female),
+ const FaIcon(FontAwesomeIcons.male, semanticsLabel: 'Male'),
+ Switch(
+ value: _gender,
+ onChanged: switchGender,
+ semanticLabel: 'Switch gender',
+ ),
+ const FaIcon(FontAwesomeIcons.female, semanticsLabel: 'Female'),📝 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.
| children: <Widget>[ | |
| FaIcon(FontAwesomeIcons.male), | |
| const FaIcon(FontAwesomeIcons.male), | |
| Switch(value: _gender, onChanged: switchGender), | |
| FaIcon(FontAwesomeIcons.female), | |
| const FaIcon(FontAwesomeIcons.female), | |
| ], | |
| children: <Widget>[ | |
| const FaIcon(FontAwesomeIcons.male, semanticsLabel: 'Male'), | |
| Switch( | |
| value: _gender, | |
| onChanged: switchGender, | |
| semanticLabel: 'Switch gender', | |
| ), | |
| const FaIcon(FontAwesomeIcons.female, semanticsLabel: 'Female'), | |
| ], |
| setUpAll(() async { | ||
| SharedPreferences.setMockInitialValues({ | ||
| 'locale': 'ar_DZ', | ||
| }); | ||
| await EasyLocalization.ensureInitialized(); | ||
| await EasyLocalization.ensureInitialized( | ||
| EasyLocalizationInMemoryStorage() | ||
| ..setLocale(value: const Locale('ar', 'DZ'))); | ||
| }); |
There was a problem hiding this comment.
Update group name and improve test isolation
Several issues need attention:
- The group name "SharedPreferences deleteSaveLocale()" is outdated as it now uses in-memory storage
- The storage state might persist between tests, potentially causing interference
Consider these changes:
- group('SharedPreferences deleteSaveLocale()', () {
+ group('EasyLocalizationStorage deleteSaveLocale()', () {
setUpAll(() async {
await EasyLocalization.ensureInitialized(
EasyLocalizationInMemoryStorage()
..setLocale(value: const Locale('ar', 'DZ')));
});
+ tearDown(() async {
+ // Reset storage state between tests
+ await EasyLocalization.ensureInitialized(EasyLocalizationInMemoryStorage());
+ });📝 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.
| setUpAll(() async { | |
| SharedPreferences.setMockInitialValues({ | |
| 'locale': 'ar_DZ', | |
| }); | |
| await EasyLocalization.ensureInitialized(); | |
| await EasyLocalization.ensureInitialized( | |
| EasyLocalizationInMemoryStorage() | |
| ..setLocale(value: const Locale('ar', 'DZ'))); | |
| }); | |
| group('EasyLocalizationStorage deleteSaveLocale()', () { | |
| setUpAll(() async { | |
| await EasyLocalization.ensureInitialized( | |
| EasyLocalizationInMemoryStorage() | |
| ..setLocale(value: const Locale('ar', 'DZ'))); | |
| }); | |
| tearDown(() async { | |
| // Reset storage state between tests | |
| await EasyLocalization.ensureInitialized(EasyLocalizationInMemoryStorage()); | |
| }); |
| await EasyLocalization.ensureInitialized(EasyLocalizationInMemoryStorage() | ||
| ..setLocale(value: const Locale('ar', 'DZ'))); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the gathered evidence, I can now provide a final response:
Test isolation needs improvement to prevent state sharing
The codebase shows multiple test groups in easy_localization_widget_test.dart using setUpAll with EasyLocalizationInMemoryStorage initialization, but no corresponding tearDown blocks for cleanup. This confirms the original concern about potential state bleeding between test groups, specifically:
- Three separate groups reuse the same storage initialization in
setUpAll- 'SharedPreferences saveLocale' (two instances)
- 'SharedPreferences deleteSaveLocale()'
The suggested refactoring in the original review comment is correct and necessary to ensure proper test isolation.
🔗 Analysis chain
Verify test isolation in setup blocks.
The storage initialization is repeated in multiple setUpAll blocks. This could potentially lead to state bleeding between test groups.
Consider refactoring to ensure proper test isolation:
- Use
setUpinstead ofsetUpAllto ensure a fresh storage instance for each test - Or clear the storage state in
tearDown
- setUpAll(() async {
+ setUp(() async {
await EasyLocalization.ensureInitialized(EasyLocalizationInMemoryStorage()
..setLocale(value: const Locale('ar', 'DZ')));
});
+ tearDown(() async {
+ final storage = EasyLocalizationInMemoryStorage();
+ await storage.clear();
+ });Also applies to: 536-537, 592-593
Let me analyze the test files further to understand the test group structure and potential state sharing.
Let me check if there are any tearDown blocks that might be handling cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all setUpAll blocks in test files
rg -A 5 "setUpAll\(" --type dart
Length of output: 5127
Script:
#!/bin/bash
# Search for test groups and their setUpAll blocks to understand isolation
rg -B 2 -A 10 "group\(.*\{" --type dart test/easy_localization_widget_test.dart
Length of output: 2107
Script:
#!/bin/bash
# Search for tearDown blocks in test files
rg "tearDown\(" -A 5 --type dart test/easy_localization_widget_test.dart
Length of output: 74
| throw StateError( | ||
| 'EasyLocalizationController not initialized. Call initEasyLocation first.'); |
There was a problem hiding this comment.
Correct method name from 'initEasyLocation' to 'initEasyLocalization'
The method name initEasyLocation seems inconsistent with the class name EasyLocalizationController. It should be initEasyLocalization to match the naming convention and avoid confusion.
Apply this diff to correct the method name and error message:
- throw StateError(
- 'EasyLocalizationController not initialized. Call initEasyLocation first.');
+ throw StateError(
+ 'EasyLocalizationController not initialized. Call initEasyLocalization first.');-static Future<void> initEasyLocation(EasyLocalizationStorage storage) async {
+static Future<void> initEasyLocalization(EasyLocalizationStorage storage) async {Update all references to this method accordingly.
Also applies to: 219-221
easy_localization_storage,easy_localization_shared_preferences_storageto extend the way locale is savedeasy_localization_helperto separate extensions used ineasy_localization_shared_preferences_storageandeasy_localizationSummary by CodeRabbit
Release Notes
New Features
easy_localization_helperandeasy_localization_storage.Bug Fixes
SharedPreferencesfor localization state management.Documentation
Tests