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 translatePlural filter #122

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add translatePlural filter #122

wants to merge 4 commits into from

Conversation

pswai
Copy link

@pswai pswai commented Nov 7, 2014

Issue #104

This adds translatePlural filter with syntax

{{'One rock' | translatePlural:rocks.length:'{} rocks'}}

This is a bit different with what @rubenv suggested

{{"1 car" | translatePlural:n:"$count cars"}}
{{"1 car" | translatePlural:n:"$count cars":"domain"}} // With a domain
  1. Use {} instead of $count. That's because the probability of collision of $count is higher than {} I think. Matching {} is also much easier.
  2. Domain is not supported as I'm not too familiar with it. It can be added later on if somebody needs it.

@rubenv
Copy link
Owner

rubenv commented Nov 7, 2014

Why not use {{$count}}? Does that get substituted in the string?

Having two different syntaxes is really troublesome: it would given an error if you have this situation:

{{"1 car" | translatePlural:n:"$count cars"}}

<div translate translate-n="n" translate-plural="{{$count}} cars">1 car</div>

Not the mismatching values for the plural string. There's no way we can handle that.

This cannot go in unless we find a way to make this work:

{{"1 car" | translatePlural:n:"{{$count}} cars"}}

@rubenv
Copy link
Owner

rubenv commented Nov 7, 2014

I just noticed that I made the mistake of putting it wrong in #104. Sorry for that.

@@ -112,6 +112,16 @@ describe("Catalog", function () {
assert.equal(catalog.getPlural(1, "There is {{count}} bird", "There are {{count}} birds", { count: 1 }), "There is 1 bird");
});

it("Can return an interpolated plural string for special {} pattern", function () {
assert.deepEqual(catalog.strings, {});
Copy link
Owner

Choose a reason for hiding this comment

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

No need to have this assertion.

@rubenv
Copy link
Owner

rubenv commented Nov 7, 2014

Apart from the points above: excellent work! If we can fix the things above, I see no reason not to merge this.

@pswai
Copy link
Author

pswai commented Nov 7, 2014

{{$count}} is not used because $parse seems to prohibit nested delimiters. Expressions like

{{'1 apple' | translatePlural:n:'{{$count}} apples'}}

result in parse error. I think that's why ngPluralize is using {} as their count token. Do you know any way to get around this?

@rubenv
Copy link
Owner

rubenv commented Nov 18, 2014

{{$count}} is not used because $parse seems to prohibit nested delimiters.
Expressions like {{'1 apple' | translatePlural:n:'{{$count}} apples'}} result in parse error.

Ok, that is a problem. Guess we'll have to take that constraint as a given.

But it's not just an option to use a new interpolation syntax as-such. Consider the case where we opt to use $count in translatePlural filters:

{{'1 apple' | translatePlural:n:'$count apples'}}

In combination with a different occurrence:

<span translate translate-n="n" translate-plural="{{$count}} apples">1 apple</span>

This would fail during message extraction since we have two different plural strings for 1 apple:

  • $count apples
  • {{$count}} apples

One option (and this might be crazy-talk) is to support $count in filters (as in the example above). This would be a special case, with two added hacks to make it work:

  1. The first step in the filter code would be to replace $count by {{$count}}, such that normal interpolation works.
  2. The extractor is modified to recognize $count in translatePlural filters and does a similar replace. That way we end up with only one plural string in the .pot: {{$count}} apples.

I'm inclined towards $count rather than {}, as it's more similar to {{$count}} and more recognisable as a variable.

Something to think about.

Adding @gabegorelick to CC, he might have input too.

@gabegorelick
Copy link
Collaborator

Yeah, $count makes more sense.

@rubenv rubenv added this to the 2.1 - Plural filters milestone Dec 8, 2014
@gabegorelick
Copy link
Collaborator

We may want to do something like rubenv/angular-gettext-tools#63 (comment) instead.

@gabegorelick
Copy link
Collaborator

FYI, you want to use $interpolate instead of $parse. That solves the {{}} issue.

Nevermind. $compile parses {{}} inside string literals.

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.

3 participants