8000 Support for ICU-based message formatting, close #256 by Kocal · Pull Request #288 · willdurand/BazingaJsTranslationBundle · GitHub
[go: up one dir, main page]
Skip to content

Support for ICU-based message formatting, close #256 #288

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Sep 19, 2020
Merged

Support for ICU-based message formatting, close #256 #288

merged 13 commits into from
Sep 19, 2020

Conversation

Kocal
Copy link
Contributor
@Kocal Kocal commented Aug 6, 2020 •
edited
Loading

PR under development, I need to update the JS part to support ICU templates.

Close #256.

I'm thinking about using intl-messageformat dependency, it provides a simple API to format one message (and not a whole catalogs like other librairies).
Example:

const IntlMessageFormat = require('intl-messageformat');

const mf = new IntlMessageFormat.IntlMessageFormat('Hello {name}!');
mf.format({ name: 'Bob' }) // Hello bob!

EDIT: See #288 (comment)

stof
stof reviewed Aug 6, 2020
View reviewed changes
@stof
Copy link
Contributor
stof commented Aug 6, 2020

what should happen is that the domain filter accepts both messages and messages+intl-icu files when being asked to filter by the messages domain.

@Kocal
Copy link
Contributor Author
Kocal commented Aug 6, 2020 •
edited
Loading

How Symfony handle messages and messages+intl-icu files? Is it possible to use "classic" and ICU message formats at the same time?

@stof
Copy link
Contributor
stof commented Aug 7, 2020 •
edited
Loading

How Symfony handle messages and messages+intl-icu files? Is it possible to use "classic" and ICU message formats at the same time?

yes it is. The opt-in is per file thanks to the suffix in the domain.

The handling in Symfony is done at the translator level (and in a few other places like the commands dealing with catalogues with support for a domain filtering): https://github.com/symfony/symfony/blob/669b3dfd42e382c61a2f540ca518d1ccb5a6ebee/src/Symfony/Component/Translation/Translator.php#L217-L221

@Kocal
Copy link
Contributor Author
Kocal commented Aug 7, 2020

Nice, thanks.

@Kocal
Copy link
Contributor Author
Kocal commented Aug 9, 2020 •
edited
Loading

I think this PR is ready to be reviewed.

The final user should have nothing to change to use translations from ICU domains, except to make library IntlMessageFormat available, see documentation.

Given the following translations files:

# translations/messages.en.yaml
hello: Hello!

# translations/messages+intl-icu.en.yaml
hello_name: Hello {name}!

This HTTP call will return translations for messages and messages+intl-icu domains:

<script src="{{ url('bazinga_jstranslation_js', { 'domain': 'messages' }) }}"></script>

And there is nothing to change with Translator.trans() calls:

Translator.trans('hello', {}, 'messages'); // Hello!
Translator.trans('hello_name', { name: 'John' }, 'messages'); // Hello John!

Like Symfony's Translator, ICU translations have an higher priority than "classic" translations.
It means that if you have two files messages.en.yaml and messages+intl-icu.en.yaml with the same translation key, the translation from messages+intl-icu.en.yaml will be used.

Note that ICU support has not been implemented in Translator.transChoice:

I've also did some improvments to the run-qunit.js file, it now display failed tests in the CLI. Otherwise it was impossible to know which tests were failing in PhantomJS (but were working in Chrome).

Travis show some failing tests, but I don't think this is related to this PR:

What do you think?
Thanks!

ping @stof @monteiro

8000
@Kocal Kocal marked this pull request as ready for review August 9, 2020 08:39
@monteiro
Copy link
Collaborator
monteiro commented Sep 8, 2020

Sorry for the late reply. Looks good to me. I need to fix the Travis tests.

@koddistortion
Copy link

Is there any chance, that this is going to be merged anytime soon?

@monteiro monteiro merged commit 43a51cf into willdurand:master Sep 19, 2020
@Kocal Kocal deleted the GH-256 branch September 19, 2020 15:56
@Kocal Kocal mentioned this pull request Oct 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for ICU-based message formatting
4 participants
@Kocal @stof @monteiro @koddistortion
0