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

Askama base crate i18n support #845

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

TTWNO
Copy link

@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
@@ -33,6 +33,7 @@ with-mendes = ["askama_derive/with-mendes"]
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

@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

@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

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 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

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
Owner

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.

@@ -33,6 +33,8 @@ pub(crate) enum Expr<'a> {
Call(Box<Expr<'a>>, Vec<Expr<'a>>),
RustMacro(Vec<&'a str>, &'a str),
Try(Box<Expr<'a>>),
#[allow(dead_code)]
Copy link

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

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

pub fn new(language: LanguageIdentifier, loader: &'static StaticLoader) -> Self {
Self { loader, language }
}

Copy link

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 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.

3 participants