kellpossible / cargo-i18n

A Rust Cargo sub-command and libraries to extract and build localization resources to embed in your application/library
MIT License
120 stars 24 forks source link

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

Closed bikeshedder closed 2 years ago

bikeshedder commented 2 years ago

This PR adds the following methods:

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.

bikeshedder commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

kellpossible commented 2 years ago

@bikeshedder would you still like me to review this?

bikeshedder commented 2 years ago

@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 commented 2 years ago

looks like i18n-embed might need a cargo fmt

bikeshedder commented 2 years ago

I removed the unused dependency itertools and reformatted the code using cargo fmt. :+1:

kellpossible commented 2 years ago

Released in i18n-embed version 0.13.4