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

I18n runtime #237

Closed
wants to merge 3 commits into from
Closed

I18n runtime #237

wants to merge 3 commits into from

Conversation

kazimuth
Copy link
Contributor

@kazimuth kazimuth commented May 5, 2019

This PR adds an I18n runtime to askama_shared. There's no codegen yet, though.

This is a little wacky because we're reimplementing some features provided by fluent. (See discussion here).

Open questions:

  • Should we add log or slog to register errors? Fluent is designed to degrade gracefully when errors are present.
  • Currently, fluent requires allocating a HashMap of arguments for every call (that takes arguments). Should we instead add a thread_local! HashMap and transparently re-use it for each call? In my benchmarks this takes ~250ns off of every call to localize (which takes ~500ns on average). However, it's a little weird for Rust.
  • I do a little bit of wacky pre-processing when choosing a locale fallback chain for a user, not sure if it's the right design. See my TODO: discuss comment in the code.

@djc
Copy link
Collaborator

djc commented May 6, 2019

Let's start without logging? We can add it later before we release this thing.

Linking to the Fluent issue on hashmap allocations again: projectfluent/fluent-rs#94. I would argue we should do without TLS for now, we can add it later.

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

Okay, wow, this looks really straightforward actually! Very nice.

Mentioned some minorish things in comments.


pub use lazy_static::lazy_static;

/// An I18n argument value. Instantiated only by the `{ localize() }` filter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to have code ordered the other way around: high-level complex dependent API up top, simple private types that don't depend on anything somewhere down the bottom. It's explained a bit more here:

https://deterministic.space/how-to-order-rust-code.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, I never thought about this. Can do.

/// This type is initialized in a lazy_static! in impl_localize!,
/// because FluentBundle can only take FluentResources by reference;
/// we have to store them somewhere to reference them.
/// This can go away once https://github.com/projectfluent/fluent-rs/issues/103 lands.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So that issue mentions some apparently newly available features/crates -- have you looked into their applicability yet? From looking over the code here, I'm mostly worried about the finer details of fallback chains. (For example, I would say that en should prefer en_US over en_AU in most projects -- though I'm not sure if the Fluent code handles it like this?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those crates don't really buy us anything, honestly. fluent-resmgr is designed for loading from the filesystem, which we don't do, and fluent-fallback does a bunch of unnecessary allocation -- re-building parts of the localization system for every call, which isn't necessary. (It's also only 50 lines of code, so i don't think it's bad to re-implement it more efficiently.)

...On the other hand, https://crates.io/crates/fluent-locale does exactly what we want here! It wasn't used in the examples before, i didn't realize it existed. Will switch to using it now :)

@kazimuth
Copy link
Contributor Author

kazimuth commented May 6, 2019

Comments addressed 🙂

@kazimuth
Copy link
Contributor Author

kazimuth commented May 6, 2019

Update: I wanna switch the runtime from concat'ing source files to including them separately, one minute...

@kazimuth
Copy link
Contributor Author

kazimuth commented May 7, 2019

Okay, I've just had a thought.

I'm looking through the rest of my i18n code, and it's occurred to me that the runtime + codegen for impl_localize aren't actually askama-specific. It's just a system for baking fluent translations into an executable, and then accessing them.

It might make sense to move most of this to its own crate, and add it to askama as a dependency.

@djc
Copy link
Collaborator

djc commented May 7, 2019

Hah, that sounds like it might make a lot of sense! I'd probably prefer for someone else to maintain that, anyway. 👍

@kazimuth
Copy link
Contributor Author

kazimuth commented May 7, 2019

Closing in favor of https://github.com/kazimuth/baked_fluent/ 🙂

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.

2 participants