Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Static typing for AssetLoader #524

Open
alexeyinkin opened this issue Sep 13, 2022 · 8 comments · May be fixed by #598
Open

Static typing for AssetLoader #524

alexeyinkin opened this issue Sep 13, 2022 · 8 comments · May be fixed by #598
Labels
enhancement New feature or request

Comments

@alexeyinkin
Copy link

alexeyinkin commented Sep 13, 2022

Currently EasyLocalization.assetLoader is dynamic.
It defaults to RootBundleAssetLoader that extends a local AssetLoader class.

easy_localization_loader has its own abstract AssetLoader, and its loaders extend it.

The following change will improve type safety:

  1. Limit EasyLocalization.assetLoader to the local AssetLoader class.
  2. Remove AssetLoader duplicate from easy_localization_loader.
  3. In easy_localization_loader, depend on easy_localization and make all loaders extend its AssetLoader.
@bw-flagship bw-flagship added the enhancement New feature or request label May 13, 2023
@bw-flagship
Copy link
Collaborator

Thanks for submitting this and explaining a solution! Maybe you want to submit a PR which implements that?

alexeyinkin added a commit to alexeyinkin/easy_localization that referenced this issue Jun 26, 2023
@alexeyinkin
Copy link
Author

@bw-flagship done.

The plan is:

  1. Merge Delete local AssetLoader class, update dependencies (#46) easy_localization_loader#50 and publish v2.0.0.
  2. Merge Static type for EasyLocalization.assetLoader (#524) #598

Note that for both packages this is a breaking change, so the bump to 4.0.0 is required. You may want to hold back publishing until you do more breaking changes and cleanup that were coming.

@bw-flagship
Copy link
Collaborator

Why did you change the file names? It is not necessary for deduplicating the AssetLoader, isn't it?

@alexeyinkin
Copy link
Author

I did not change them but sorted alphabetically. There is a lint for this: https://dart-lang.github.io/linter/lints/directives_ordering.html
Same goes for removing the library directive: https://dart.dev/effective-dart/style#dont-explicitly-name-libraries

I just did a quick scan of what I can easily fix. I can undo that if you want.

@bw-flagship
Copy link
Collaborator

I meant this breaking change:

- **BREAKING**: `JsonAssetLoader`, `XmlAssetLoader`, and `YamlAssetLoader` now use `_` instead of `-` when converting a locale to a file name.

Why is this change necessary?

@alexeyinkin
Copy link
Author

These loaders were using localeToString function to convert from a Locale to the file name component:
https://github.com/aissat/easy_localization_loader/blob/0469058b11f65b92c4fdc7a2cbbf1900401df0d6/lib/src/yaml_asset_loader.dart#L12

This function was defined in both easy_localization_loaders and easy_localization, and is deprecated in the latter. The reason for deprecation is not mentioned, but my guess was that the author wanted to encourage more consistent naming of files, like en_US.yaml instead of en-US.yaml, since _ is the standard in Dart.

The straightforward way to keep the old behavior was to use the deprecated function from easy_localization, which I did not like. Following my guess of the purpose of the deprecation, I renamed the files.

When you asked, I discovered that Locale.toStringWithSeparator was introduced instead. So if we want to preserve the behavior, I can switch to using it.

Still the _ is more straightforward separator, and we can switch to defaulting to it at some point later.

@bw-flagship
Copy link
Collaborator

Thanks for explanation. I would prefer to preserve the old behavior to reduce breaking changes to a minimum :)

alexeyinkin added a commit to alexeyinkin/easy_localization that referenced this issue Jun 27, 2023
@alexeyinkin
Copy link
Author

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants