Derive i18n support #844
Derive i18n support #844
Conversation
added fail() if localize() is detected in a template while the localization feature is not activated
…ich tries until on parser succeeds but this also means when localize gets call with non localize string it fails and generates a failure This reverts commit 355163a.
|
Thanks for splitting this up! Unfortunately I just merged #834, which will require some changes here. Also, please fold the Cargo formatting changes and the removal of |
|
Ping @TTWNO. Do you think you will have time to come back to this? It'd be a shame that this great feature wouldn't be merged. :) |
|
Ah! Totally forget. Let me see what I can do this afternoon. |
|
I... think I fixed the conflicts and still integrated it properly. Let me know if this needs some additional work. I realize making |
|
I fixed the formatting. The warnings are mostly a big bunch of functions that are not run. IIUC, I'm not actually calling the Anyways, this is really close, and I would appreciate a review on what I need to fix to get this over the finish line. Some dependency depends on a later version of |
|
I am still committed to getting this merged. My biggest issue is that I do not often have time to work on this in big blocks (20 minutes here, an hour there) and so it ends up languishing in "I'll get to it" hell. I'll emphasize again that a bit of TLC would be super helpful in getting this wrapped up. If anyone is willing to review and give this some help, I will get on at least once a week to read and make changes as necessary. I am still using my old, branched version in an activate project to have internationalization support. It would be super helpful to start using a mainline version of askama :) |
|
That's still 90 commits. Can you squash a bit please? ^^' |
There was a problem hiding this comment.
No need to squash this for now, let's just review it as one big commit because it looks like that might make sense for this. The test case looks pretty good and a lot of the other stuff does, too, but there are still seems to be a bunch of unnecessary complexity.
| use askama::i18n::{langid, Locale}; | ||
| use askama::Template; | ||
|
|
||
| askama::i18n::load!(LOCALES); |
There was a problem hiding this comment.
I think this should look like static LOCALES: _ = askama::i18n::load!(); -- what is preventing us from doing something more like that?
There was a problem hiding this comment.
this is the expansion of line 6:
static LOCALES: ::askama::i18n::fluent_templates::once_cell::sync::Lazy<
::askama::i18n::fluent_templates::StaticLoader,
> = ::askama::i18n::fluent_templates::once_cell::sync::Lazy::new(|| {
mod fluent_templates {
pub use ::askama::i18n::fluent_templates::*;
pub mod once_cell {
pub mod sync {
pub use ::askama::i18n::Unlazy as Lazy;
}
}
}
pub static LOCALES: fluent_templates::once_cell::sync::Lazy<
fluent_templates::StaticLoader,
> = fluent_templates::once_cell::sync::Lazy::new(|| {
static CORE_RESOURCE: fluent_templates::once_cell::sync::Lazy<
Option<fluent_templates::fluent_bundle::FluentResource>,
> = fluent_templates::once_cell::sync::Lazy::new(|| { None });
.......and so on
...so the expansion handles the variable setting. Because the type can't be elided (it being static), that would be requiring the end user to write like so:
static LOCALES: askama::18n::fluent_templates::once_cell::Lazy<askama::i18n::fluent_templates::StaticLoader> = askama::i18n::load!();
// with sane re-exports
static LOCALES: askama::i18n::LazyStaticLoader = askama::i18n::load!();OP is being consistent, in a sense, with the pattern used by the wrapped fluent_templates::static_loader macro, which is documented like so:
//! ### Basic Example
//! ```
//! fluent_templates::static_loader! {
//! // Declare our `StaticLoader` named `LOCALES`.
//! static LOCALES = {
//! // The directory of localisations and fluent resources.
//! locales: "./tests/locales",
//! // The language to falback on if something is not present.
//! fallback_language: "en-US",
//! // Optional: A fluent resource that is shared with every locale.
//! core_locales: "./tests/locales/core.ftl",
//! };
//! }
//! # fn main() {}
//! ```
...in our case, though, the wrapper handles the locales and other settings by means of reading the i18n.toml file, if i'm reading things correctly
I think it would be worth some follow-up after this PR is merged to see if it would be nice to "de-abstract" a touch the underlying fluent engine. The scenario I have in mind is that an "actix-web + askama" project should be internationalised by turning it into an "actix-web + askama + fluent" and not "actix-web + fluent + askama-with-i18n". That probably doesn't make sense, but this conversation is a bit out of scope, so I'll clarify when it comes time to put on the polish.
There was a problem hiding this comment.
I think static LOCALES: askama::i18n::LazyStaticLoader = askama::i18n::load!(); looks better, I prefer it over the more opaque/comprehensive wrapper macro.
| @@ -0,0 +1,425 @@ | |||
| use fluent_templates::LanguageIdentifier; | |||
There was a problem hiding this comment.
We should not duplicate all this stuff across the derive and parser crates.
| #[cfg(not(feature = "i18n"))] | ||
| fn localize(i: &'a str) -> ParseResult<'a, Self> { | ||
| let (i, _) = pair(tag("localize"), ws(tag("(")))(i)?; | ||
| eprintln!(r#"Activate the "i18n" feature to use {{ localize() }}."#); |
There was a problem hiding this comment.
This should not be an eprintln!(), but some form of error that is yielded to the caller. (Similar for the other error conditions here.)
There was a problem hiding this comment.
like so?
#[cfg(not(feature = "i18n"))]
fn localize(i: &'a str) -> ParseResult<'a, Self> {
+ // Happy path: no i18n _should_ fail to parse "localize ("
let (i, _) = pair(tag("localize"), ws(tag("(")))(i)?;
- eprintln!(r#"Activate the "i18n" feature to use {{ localize() }}."#);
- Err(nom::Err::Failure(error_position!(i, ErrorKind::Tag)))
+
+ Err(nom::Err::Failure(ErrorContext {
+ input: i,
+ message: Some(Cow::Owned(format!(
+ r#"Activate the "i18n" feature to use {{ localize() }}."#
+ )))
+ }))
}There was a problem hiding this comment.
Yup, that looks reasonable to me!
| fluent-syntax = { version = "0.11.0", default-features = false, optional = true } | ||
| fluent-templates = { version = "0.8.0", default-features = false, optional = true } | ||
| serde = { version = "1.0.193", default-features = false, features = ["derive"], optional = true } | ||
| basic-toml = { version = "0.1.7", default-features = false, optional = true } | ||
| syn = { version = "2.0.43", default-features = false } | ||
| proc-macro2 = { version = "1.0.71", default-features = false } |
There was a problem hiding this comment.
These should be ordered alphabetically -- but also, why do we need all this extra stuff in the parser crate? Maybe this was misrebased?
| } | ||
|
|
||
| fn get_i18n_config() -> Result<&'static Configuration, CompileError> { | ||
| lazy_static! { |
There was a problem hiding this comment.
We should not use lazy_static!(), instead use a OnceLock or once_cell::Lazy.
There was a problem hiding this comment.
standby for pr update:
@@ -1,4 +1,4 @@
-use fluent_templates::LanguageIdentifier;
+use fluent_templates::{once_cell, LanguageIdentifier };
use std::borrow::Cow;
use std::collections::hash_map::Entry;
use std::collections::{HashMap, HashSet};
@@ -259,12 +259,9 @@ fn read_configuration() -> Result<Configuration, String> {
})
}
-use fluent_templates::lazy_static::lazy_static;
fn get_i18n_config() -> Result<&'static Configuration, CompileError> {
- lazy_static! {
- static ref CONFIGURATION: Result<Configuration, String> = read_configuration();
- }
+ static CONFIGURATION: once_cell::sync::Lazy<Result<Configuration, String>> = once_cell::sync::Lazy::new(read_configuration);| Ok(Some((language, resources))) | ||
| } | ||
|
|
||
| fn read_configuration() -> Result<Configuration, String> { |
There was a problem hiding this comment.
Could this configuration be part of askama.toml? I don't see a good reason why it should be a separate file?
| /// | ||
| /// Rationale: StaticLoader cannot be cloned. | ||
| #[doc(hidden)] | ||
| pub struct Unlazy<T>(parking_lot::Mutex<UnlazyEnum<T>>); |
There was a problem hiding this comment.
Why do we need this? The stated rationale is that StaticLoader cannot be cloned, but why would it need to be?
| @@ -23,3 +22,5 @@ default-members = [ | |||
| "askama_parser", | |||
| "testing", | |||
| ] | |||
|
|
|||
| resolver = "2" | |||
| pub struct Locale<'a> { | ||
| loader: &'a StaticLoader, | ||
| language: LanguageIdentifier, | ||
| } |
There was a problem hiding this comment.
Would it be possible to also support non-static loaders (e.g ArcLoader)? Could be a dyn type or parameterised by the type. Wondering as I have a use-case for this
There was a problem hiding this comment.
I'm adding some resource files at runtime:
https://github.com/IsiXhosa-click/isixhosa_click/blob/translations/server/src/i18n.rs#L26-L53
The app server can support running as one of a distinct set of 'sites' depending on a command line flag - this simplifies code & deployment. Each site has its own name, description, purpose, and other translatable values which are also used by the common translation resources. Because switching is done at runtime using a command-line flag, I can't use a static loader.
For instance, the value of source-language as used in this message in the main bundle
about = -snip-
.description = { site.short-name } is a free, open, online dictionary for { target-language } and { source-language }.depends on which site is being used. If the site is "isixhosa" and the user's language is en-ZA, it will use this value from the site-specific bundle:
source-language = EnglishIf the site is "isixhosa" and the user's language is xh, then it will use the appropriate one from the site-specific bundle:
source-language = IsiNgesiFinally, if the site is "isizulu" and the locale en-ZA, then it will use this value:
source-language = English(It happens to be the same for the isixhosa site in en-ZA, but this isn't the case for all strings)
There was a problem hiding this comment.
Okay, I think that all sounds plausible, but I'd probably like to declare it out of scope for the initial PR.
An smaller PR, with basic functionality of #841 , but with some important differences:
i18ndependencies are feature gated. TheExprtype will always contain aLocalizationvariant, but the developer will still be given an error if they attempt to usei18nwithin a template without the feature enabled.i18ninto other parts of the system. This will be for a future PR, since djc wants smaller, more manageable PRs for this large feature.Unlazytype, implemented withparking_lotand anunsafe { transmute ... }; whether or not its supposed "unsafely" is really an issue is not my main concern. Rather, creating a new type, just to implement one singular function seems excessive. Perhaps I'm wrong about this, and I'm happy to revert that change upon request.Over half the code, 393 lines, is in the
src/i18n.rsfile, all other modifications are fairly small and simply introduce the ability to handle localization expressions.This is the biggest PR by far, since all functions within
src/i18n.rsare required to get the expressions working correctly.