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

Closed
wants to merge 70 commits into from
Closed

I18n #700

wants to merge 70 commits into from

Conversation

89Q12
Copy link

@89Q12 89Q12 commented Jun 17, 2022

This PR is based on the work of jhoobergs and PR #434.
I mainly just updated the code to work with the changes that have been made since the initial PR, and I split it into its own feature "localization" which can be renamed to maybe "with-localization".
Also I added askama::Locale as suggested in #434.

Pending:

  • Docs
  • Improve the feature, mainly the use of #[cfg(feature = "localization")] since I'm new to rust/cargo

All tests pass.
Looking at the old PR the TODO: "Fix the test for invalid text_id's" isn't really needed since we make use of a function from fluent that checks for a fallback language which is why the test doesn't panic, in fact it will never panic as seen here

askama_derive/src/input.rs Outdated Show resolved Hide resolved
@LeoniePhiline
Copy link

LeoniePhiline commented Oct 11, 2022

CI fails due to a newly introduced match at Add Expr::is_cachable().
The i18n branch is currently outdated and must be rebased.

@11Tuvork28 Could you rebase your branch on top of the current main? I'll then mark Expr::Localize(_, _) as cacheable via recursive arguments check.

89Q12 and others added 6 commits October 11, 2022 17:14
docs: Rename example template struct
Quote from https://projectfluent.org/fluent/guide/hello.html

```ftl
hello = Hello, world!
```

Each message has an identifier that allows the developer to bind it to the place in the software where it will be used. The above message is called `hello`.
fix: Implement missing recursive `is_cachable()` for `Expr::Localize`
@LeoniePhiline
Copy link

I implemented missing recursive is_cachable() for Expr::Localize after the fork was updated.

Would a maintainer please be so kind and approve the workflow? Thank you!

@LeoniePhiline
Copy link

LeoniePhiline commented Oct 11, 2022

Please excuse, my mistake. I had missed to check compilation with default features (without i18n).

This is fixed now in 89Q12#7, waiting to be merged into this PR branch.

Once 89Q12@2bbba70 has landed in this PR, the checks should succeed.

Edit: Landed - all checks pass now.

Fix: Add missing trailing newline in ftl file
@djc
Copy link
Owner

djc commented Oct 11, 2022

@LeoniePhiline and @11Tuvork28 thanks for your patience with me and for the hard work on getting this ready. I'll try to make another pass soon, but feel obliged to warn you that I have a very busy week and generally a lot on my plate.

@TTWNO
Copy link

TTWNO commented Apr 17, 2023

I'd like to pop in on this issue and ask if anything needs to be done to get this merged?

I'm writing a website for my hockey league that needs pretty advanced i18n that fluent provides; I have some time, so please let me know what needs to be done.

@89Q12
Copy link
Author

89Q12 commented Apr 20, 2023

I'd like to pop in on this issue and ask if anything needs to be done to get this merged?

I'm writing a website for my hockey league that needs pretty advanced i18n that fluent provides; I have some time, so please let me know what needs to be done.

Rebase for one would be good, then I'm not sure that this is even wanted anymore given that this hasn't gotten any anttention.

@djc
Copy link
Owner

djc commented Apr 20, 2023

Again, sorry to everyone following this PR for my lack of feedback here. Please be assured that I think about this PR at least once a week, but in its current state it's hard for me to justify taking out the time to review the large amount of changes here, compared to a bunch of other stuff (roughly across my family, my day job and other open source projects that I help maintain).

I guess the main thing that could help resolving this deadlock would be to represent the changes here as a series of a smaller PR that have one logical change per PR, so that I can review each of them quickly and thus we can make progress. I should probably have given this feedback sooner, but I figured I'd be able to get to it sooner.

I hope that someone is willing to take that one -- I really would like to get this work merged -- and thanks to everyone who has worked on it so far.

@89Q12
Copy link
Author

89Q12 commented Apr 20, 2023

Thanks for the feedback!!
Much appreciated, I will try to take some time and break down the changes into logical blocks.
And no worries about not getting to review stuff, its understandable we all have only 24 hours per day.

@TTWNO
Copy link

TTWNO commented Apr 20, 2023

If you want to see a recently-rebased version you can take a look at my fork.

@89Q12
Copy link
Author

89Q12 commented Apr 20, 2023

If you want to see a recently-rebased version you can take a look at my fork.

Can you PR that to my fork? Would be sweet :)
But I will take a look regardless, maybe we could work together on splitting it up? Would make it fast and more doable for me

@djc
Copy link
Owner

djc commented Apr 20, 2023

You can't really PR a rebase. However, as the PR author you can reset to the rebased commit/branch.

@TTWNO TTWNO mentioned this pull request Jul 16, 2023
@mgmuesuu

This comment was marked as spam.

@89Q12
Copy link
Author

89Q12 commented Dec 18, 2023

Closing in favor of #841 and its related PRs.
Thanks again to everyone involved.

@89Q12 89Q12 closed this Dec 18, 2023
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.

8 participants