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

Implement FluentLanguageLoader::get_lang(...) methods #84

Merged
merged 4 commits into from
Feb 23, 2022

Conversation

bikeshedder
Copy link
Contributor

This PR adds the following methods:

  • get_lang
  • get_lang_args
  • get_lang_args_concrete
  • get_lang_args_fluent

Those methods work exactly like their non-lang counterparts
but add a lang argument that can be used to specify a list of
language codes without needing to change the global current
language setting.

This closes #59.

This adds the following methods:

- get_lang
- get_lang_args
- get_lang_args_concrete
- get_lang_args_fluent

Those methods work exactly like their non-lang counterparts
but add a lang argument that can be used to specify a list of
language codes without needing to change the global current
language setting.

This closes kellpossible#59.
@bikeshedder
Copy link
Contributor Author

While at it I also refactored all the other get(...) methods and removed some superfluous calls of HashMap::new and Vec::new in the process. Calling the get(...) methods now results in less allocations.

I was wondering about the original implementation of get_args. I just replaced all conversion from HashMap to FluentArgs by a unified hash_map_to_fluent_args function making the code a lot leaner and easier to reason about.

@bikeshedder
Copy link
Contributor Author

@kellpossible I don't really like the interface of this. What do you think about adding a IntoFluentArgs trait and deprecating the *_args_concrete and *_args_fluent methods in the process.

If you agree with that change I'd change that PR so it doesn't add the get_lang_args_concrete and get_lang_args_fluent methods but replaces it by a more flexible get_lang_args method using this new IntoFluentArgs trait.

@bikeshedder
Copy link
Contributor Author

@saskenuba Could you please review this PR? I ended up going a different route and implementing get_lang* methods in the FluentLanguageLoader directly as it resulted in fewer code duplications. In order to implement the CurrentLanguage trait (see #82) this feature needs to be part of the existing loader anyways.

@bikeshedder
Copy link
Contributor Author

The CI failures have nothing to do with the actual code changes. 🤔

@kellpossible
Copy link
Owner

@bikeshedder would you still like me to review this?

@bikeshedder
Copy link
Contributor Author

@bikeshedder would you still like me to review this?

Having that merged doesn't hurt as it adds a important feature without breaking any existing code.

I'm still looking into implementing #82, but tbh. that's an entirely different beast as it requires some major refactoring.

@kellpossible
Copy link
Owner

kellpossible commented Feb 22, 2022

looks like i18n-embed might need a cargo fmt

Copy link
Owner

@kellpossible kellpossible left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just the code format and question about itertools

@@ -24,6 +24,7 @@ fluent-syntax = { version = "0.11", optional = true }
gettext_system = { package = "gettext", version = "0.4", optional = true }
i18n-embed-impl = { version = "0.8", path = "./i18n-embed-impl", optional = true }
intl-memoizer = "0.5"
itertools = "0.10"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used that in an earlier version but could then use the builtin .chain() method. It's not being used.

@bikeshedder
Copy link
Contributor Author

I removed the unused dependency itertools and reformatted the code using cargo fmt. 👍

@kellpossible kellpossible merged commit aa84e6c into kellpossible:master Feb 23, 2022
@kellpossible
Copy link
Owner

Released in i18n-embed version 0.13.4

kellpossible pushed a commit that referenced this pull request Jan 14, 2023
Adds a single new method lang.

This methods allows creating a shallow copy of the
FluentLanguageLoader which can than be used just like the original
loader but with a different current language setting. That makes it
possible to use the fl! macro without any changes and is a far more
elegant implementation than adding multiple get_lang* methods as
done in #84.

Co-authored-by: Michael P. Jung <[email protected]>
kellpossible added a commit that referenced this pull request Jan 14, 2023
Re-implementation of #59 (a rebase and cleanup of #88)

Adds a single new method lang.

This methods allows creating a shallow copy of the
FluentLanguageLoader which can than be used just like the original
loader but with a different current language setting. That makes it
possible to use the fl! macro without any changes and is a far more
elegant implementation than adding multiple get_lang* methods as
done in #84.

Co-authored-by: Michael P. Jung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

i18n-embed: get message_id for desired locale, falling back directly to base fallback language
2 participants