Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

Askama base crate i18n support#845

Open
TTWNO wants to merge 14 commits into
askama-rs:mainfrom
TTWNO:askama_base_crate_i18n_support
Open

Askama base crate i18n support#845
TTWNO wants to merge 14 commits into
askama-rs:mainfrom
TTWNO:askama_base_crate_i18n_support

Conversation

@TTWNO
Copy link
Copy Markdown

@TTWNO TTWNO commented Jul 30, 2023

Depends on #844 :

  • Adds Locale struct behind feature flag,
  • Does not include unsafe code, unlike the original I18n #700 PR
  • This also drops a dependency on parking_lot, since it was used as part of the unsafe code
  • 113 line diff between this PR and Derive i18n support #844

@TTWNO TTWNO mentioned this pull request Jul 30, 2023
Comment thread askama/Cargo.toml
with-rocket = ["askama_derive/with-rocket"]
with-tide = ["askama_derive/with-tide"]
with-warp = ["askama_derive/with-warp"]
i18n = ["askama_derive/i18n", "fluent-templates"]
Copy link
Copy Markdown

@Ben-PH Ben-PH Feb 6, 2024

Choose a reason for hiding this comment

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

as this is a fluid backed i18n, would it be appropriate to self-document that in the feature name? e.g. i18n_fluent?

Copy link
Copy Markdown

@Ben-PH Ben-PH left a comment

Choose a reason for hiding this comment

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

I'm by no means a heavy-hitter. I'm just passing through in my search of "the good way" to add i18n to my askama project, and thought I'd try to do my little part.

nothing in this review should be considered blocking. I didn't see any issues, but that by no means implies I didn't miss any. I hope this helps. I really look forward to askama + fluent being able to Just Work^tm

Comment on lines +1401 to +1404
for (k, v) in args {
buf.write(&format!("({:?}, ::askama::i18n::FluentValue::from(", k));
self.visit_expr(buf, v)?;
buf.writeln(")),")?;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

personal preference for key and val

Comment thread askama_derive/src/lib.rs
Comment on lines +25 to +32
#[cfg(feature = "i18n")]
match i18n::load(_input) {
Ok(ts) => ts,
Err(err) => err.into_compile_error(),
}

#[cfg(not(feature = "i18n"))]
CompileError::from(r#"Activate the "i18n" feature to use i18n_load!()."#).into_compile_error()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

consider using cfg-if

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If you mean the crate, I'd prefer not to add more dependencies unless we really need them.

Call(Box<Expr<'a>>, Vec<Expr<'a>>),
RustMacro(Vec<&'a str>, &'a str),
Try(Box<Expr<'a>>),
#[allow(dead_code)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it feasible to feature gate this instead?

Comment on lines +339 to +346
#[cfg(not(feature = "i18n"))]
fn expr_localize(i: &str) -> IResult<&str, Expr<'_>> {
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)))
}

#[cfg(feature = "i18n")]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

consider using cfg-if crate

Comment thread askama/src/i18n.rs
pub fn new(language: LanguageIdentifier, loader: &'static StaticLoader) -> Self {
Self { loader, language }
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Doc-comment here would be handy.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants