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

Add fluent builtin Tera function. #1040

Closed
wants to merge 13 commits into from
Closed

Conversation

XAMPPRocky
Copy link
Contributor

This adds a new tera builtin function fluent function if there's fluent localisations directory. Obviously more documentation, and tests are needed, but I wanted to open a PR to discuss how the feature should work before doing that.

Tera Example

{% fluent(lang="en-US", key="menu-item") %}

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

  • Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • Have you created/updated the relevant documentation page(s)?

@Keats
Copy link
Collaborator

Keats commented May 26, 2020

How does passing args work?

@XAMPPRocky
Copy link
Contributor Author

Oh yes sorry. It works as you would expect. You name the arguments in the function call and they are converted to arguments for fluent. I've linked the tera function implementation below.

{% fluent(lang="en-US", key="menu-item", arg="hello") %}

https://github.com/XAMPPRocky/fluent-templates/blob/d1c4dde6d6916b8b050a20507aa28c64672464b9/src/helper/tera.rs#L38-L68

@XAMPPRocky XAMPPRocky force-pushed the fluent branch 4 times, most recently from e7e57d4 to 4400d9a Compare June 8, 2020 13:37
@XAMPPRocky
Copy link
Contributor Author

@Keats Hey, sorry for not getting back to this sooner, I was polishing up a new release of the underlying library that should now have more documentation on how it works.

I'm unsure about what tests need to be added and where to add documentation for this feature in Zola If you could give me any advice on what you think needs to be added for this be ready to be merged, I would really appreciate it!

@Keats
Copy link
Collaborator

Keats commented Jun 9, 2020

I need to think a bit on the best way to integrate fluent to Zola. I think the easiest test would be to add translations to the test_site_i18n site and check that it works well but we do need to figure out the structure etc
Any thoughts on that? The current translations in config.toml approach need to be ripped out too

@XAMPPRocky
Copy link
Contributor Author

XAMPPRocky commented Jun 10, 2020

So I personally don't know if there's a lot that needs to change on Zola's side. The translations configuration and trans function should eventually be removed but not in this PR. I think there should be some kind of grace period where their use triggers a warning to give users time to switch.

Aside from that I want to make a few additions to this PR which is changing languages to use unic_langid::LanguageIdentifier. I was also thinking of maybe having Zola warn when you have locales in your fluent directory that haven't been specified in your config.toml.

Another thing that I'm not sure if we want to include in this PR or separately would be to add --draft languages. As using fluent will enable using team projects like pontoon, and projects like the rust-lang website have localisations done solely by volunteers that could take months to a year for those localisations to be ready for public consumption, so being able to keep them in the codebase without being public while they're being worked on would be great.

I was thinking of just adding a draft = true key to the languages object. E.g.

languages = [
    { code = "fr", draft = true },
    { code = "de" },
]

One thing to consider is how to make this work with the current blog content localisation system (index.fr.md), as I think we'd want to keep that. Having both would let us easily translate the website and the blog on rust-lang. I'll need think about that more.

@Keats
Copy link
Collaborator

Keats commented Jun 12, 2020

Draft languages can definitely be added later on.
Something that would be very useful to test is to create or find a realistic site/blog (if anyone wants to volunteer their site as well!) and see how this works in practice. I think it doesn't require much change too but would like to be sure we don't have some blind spots.

@XAMPPRocky
Copy link
Contributor Author

@Keats Im currently preparing a new release of tokei at the moment but once that’s finished I’ll polish up this PR and make a post on the Zola forums asking people to try it out.

@Keats
Copy link
Collaborator

Keats commented Jun 29, 2020

Thanks for tokei by the way!

@BertalanD
Copy link

Have you guys have an ETA on this? Is there any way I can help?

@XAMPPRocky
Copy link
Contributor Author

Hey, unfortunately I've had less time than I've hoped and there's some migration work to implement this right, so for now I'm going to close this PR. @BertalanD If you'd like to continue on the PR I'd be happy to help out with documentation, answer any questions, and review code. I've listed the details of what I think is required for this feature to be ready.

  • Migrate Zola's language identifier (such as en-US) from using String to using unic_langid::LanguageIdentifier.
  • Use fluent-templates to add the tera function. (Essentially this PR's changes)
  • Add tests
  • Add user level documentation.

@BertalanD
Copy link

BertalanD commented Jul 29, 2020

Should the user be able to override Fluent messages of themes?

I think we also need to set a clear hierarchy for Fluent resources, like:

locales
├── common.ftl
├── en-US
│   └── index.ftl
├── en
│   ├── index.ftl
|   └── page.ftl
└── fr
    ├── index.ftl
    └── page.ftl

The algorithm for determining which Fluent resources get loaded should be the following:

  • exact language, exact template name,
  • partial language match, exact template name,
  • exact language, parent template,
  • partial language match, parent template,
  • language-specific common.ftl (a template named common might exist, so we need to find a better name),
  • common.ftl

A template author should not need to duplicate all of the messages just to account for slight regional differences in spelling. We should use fluent-langneg. The language-agnostic common.ftl would be where template name, authors, and copyright info are set.

@XAMPPRocky fluent_templates should take a list of ftl files, in order of preference. The parsed resources would need to be cached for performance reasons, because this fallback chain needs to be established for every template invocation.

In the example above, the template index.html would load, in order of preference

  • en-US/index.ftl, en/index.ftl, common.ftl for the locale en-US,
  • en/index.ftl, common.ftml for en,
  • fr/index.ftl, common.ftl for fr

The template page.ftl that extends page.ftl would load

  • en/page.ftl, en-US/index.ftl, en/index.ftl, common.ftl for en-US,
  • en/page.ftl, en/index.ftl, common.ftl for en,
  • fr/page.flt, fr/index.ftl, common.ftl for fr

@XAMPPRocky XAMPPRocky reopened this Jul 29, 2020
XAMPPRocky pushed a commit to XAMPPRocky/zola that referenced this pull request Jul 29, 2020
This change was proposed by XAMPPRocky in getzola#1040.
@XAMPPRocky
Copy link
Contributor Author

@BertalanD It seems there's some merge conflicts. I created a rebased version at https://github.com/XAMPPRocky/zola/blob/fluent-rebased have a look and if it's good I can push it to here.

@BertalanD
Copy link

Sure, go ahead.

components/site/src/feed.rs still uses strings for language, so I'll fix it.

@BertalanD
Copy link

BertalanD commented Jul 30, 2020

Here's a quick to-do

  • Use unic_langid::LanguageIdentifier for languages
  • Implement basic fluent function
  • Either add deprecation warnings for languages or make it useful
  • Docs, docs, and even when your eyes start to bleed, still docs
  • Test everything
  • Load localizations for themes. @XAMPPRocky: ArcLoader should be able to load from multiple directories
  • .ftl file hierarchy, as described in my previous comment

@XAMPPRocky
Copy link
Contributor Author

@BertalanD Thank you for writing this, and I agree with that general direction however I think we should separate some of those items as I don't think they are all required for this to be ready to be merged versus being "feature complete". For example I think it's okay if the algorithm for loading locales isn't perfect at first (I'm also not sure which parts of it are already implemented by fluent-templates), or if the built-in templates don't have localisation. All of that can be future work.

@Keats
Copy link
Collaborator

Keats commented Jul 30, 2020

Live reload/rebuild for locale files

Don't worry about that bit, the reload system is going to change as soon as I have time ™️
It will rebuild everything instead of trying to be smart.

@BertalanD
Copy link

BertalanD commented Aug 4, 2020

Writing this up as a TO-DO for myself but I'm curious if it seems reasonable.

We should probably add an alias field to Language to not break links to pages that aren't using the canonical syntax. Filenames would be matched against it and it will also be used in the output. Meanwhile, code would be passed to templates or functions. This could also be used for using a language variant for translations meanwhile keeping the language names short. (i.e. en could refer to en-GB and the templates would be rendered with the British spelling.)

I need help with making fluent-templates load translations supplied for the theme. In my opinion, it's a crucial feature because the l10n system will be used the most in themes anyway.

Which theme should I start porting over to the new localization system? I have the time to work on something that isn't available for Zola yet.

@XAMPPRocky XAMPPRocky closed this Aug 21, 2020
@XAMPPRocky
Copy link
Contributor Author

Going to close this for now as it is now out of sync with the main branch.

@XAMPPRocky
Copy link
Contributor Author

@BertalanD

I need help with making fluent-templates load translations supplied for the theme. In my opinion, it's a crucial feature because the l10n system will be used the most in themes anyway.

Could you open an issue for what you think is needed on the fluent-templates side and we can discuss there?

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.

9 participants