askama icon indicating copy to clipboard operation
askama copied to clipboard

Derive i18n support

Open TTWNO opened this issue 1 year ago • 7 comments

An smaller PR, with basic functionality of #841 , but with some important differences:

  1. Only sections of code that use the optional i18n dependencies are feature gated. The Expr type will always contain a Localization variant, but the developer will still be given an error if they attempt to use i18n within a template without the feature enabled.
  2. Only the basic, underlying derivations are implemented in this PR. This does not connect i18n into other parts of the system. This will be for a future PR, since djc wants smaller, more manageable PRs for this large feature.
  3. Removes the use of the Unlazy type, implemented with parking_lot and an unsafe { 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.rs file, 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.rs are required to get the expressions working correctly.

TTWNO avatar Jul 30 '23 18:07 TTWNO

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 Unlazy into the originating commits.

djc avatar Jul 31 '23 08:07 djc

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

GuillaumeGomez avatar Dec 10 '23 13:12 GuillaumeGomez

Ah! Totally forget.

Let me see what I can do this afternoon.

TTWNO avatar Dec 10 '23 23:12 TTWNO

I... think I fixed the conflicts and still integrated it properly.

Let me know if this needs some additional work. I realize making askama_parse optionally dependent on fluent-* is not ideal.

TTWNO avatar Dec 27 '23 06:12 TTWNO

I fixed the formatting. The warnings are mostly a big bunch of functions that are not run. IIUC, I'm not actually calling the exp_localization anywhere, or if I am, it can not be triggered.

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 tinystr than the MSRV for the crate. It's two versions ahead 1.67 vs 1.65. I'd like some direction on whether explicitly downgrading the dependency is better, or if the MSRV of askama should be raised.

TTWNO avatar Jan 07 '24 20:01 TTWNO

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

TTWNO avatar Jan 07 '24 20:01 TTWNO

That's still 90 commits. Can you squash a bit please? ^^'

GuillaumeGomez avatar Jan 08 '24 09:01 GuillaumeGomez