Improve fallback

[crissdev]

Closes #210

@rubenv Let me know if I should add more tests. The implementation is pretty straightforward.

[rubenv]

Pretty much what I had in mind!

One more needed change: it needs to use the fallback language in getPlural: a plural form lookup for ru_RU will return the default (English) version, instead of what the ru rules define.

[rubenv]

And with that in mind, it's probably better to name it baseLanguage or similar.

As in "Dutch is the base language for Dutch (Netherlands)". :-)

[crissdev]

Some notes about the changes:

• baseLanguage is private because it conflicts with catalog's attribute with the same name. I noticed in the docs it's now deprecated. I think with a 3.0 release this method can be public and replace the attribute.
• plural detection is pretty hard-coded (see the if statement in the getPlural method - there's no clear way to know if plural is defined or not since for every language that's not defined a default value is returned. To make things even difficult a plural form is defined for pt_BR. A solution would be to change the generated output to allow more control. Any thoughts on this?

[CrlsMrls]

+1 please merge :)

[rubenswieringa]

@crissdev / @rubenv what's the status of this? Also, should there maybe be a test that checks that, when a language and its fallback both have a translation, the former its translation is returned?

[rubenv]

Had a quick look at this (thanks for the ping @rubenswieringa).

One way to make this cleaner is to add an extra function getStringFormFor(lang, string, n, context) (suggestions for a better name welcome). This could then be used by getStringForm():

getStringForm: function (string, n, context) {
    return this.getStringFormFor(this.currentLanguage, string, n, context) ||
           this.getStringFormFor(this.baseLanguage(), string, n, context);
}

The call to gettextPlurals would then have to move from getPlural to inside getStringFormFor, so that each language can have different rules.

This would reduce the need to hardcode pt_BR and it wouldn't require changes to gettextPlurals.

A fallback here would amount to:

// pt_BR    Brazilian Portuguese  nplurals=2; plural=(n > 1);
// pt       Portuguese            nplurals=2; plural=(n != 1);
// Assuming n = 0
return this.getStringFormFor("pt_BR", string, n, context) || // gettextPlurals("pt_BR", n) -> 0
       this.getStringFormFor("pt", string, n, context);      // gettextPlurals("pt", n) -> 1

Does that make sense?
Anyone want to take this up?

[rubenswieringa]

@rubenv I'm looking into it, if I manage to come up with anything I'll open up a new PR.

[rubenswieringa]

The call to gettextPlurals would then have to move from getPlural to inside getStringFormFor,

@rubenv are you sure we can move that function-call from getPlural() to getStringFormFor()?

When getStringForm() gets called, it expects that its n-parameter is a value that has already been modified by gettextPlurals(). Then it would call getStringFormFor(), which would also call gettextPlurals(). So gettextPlurals() would be called twice – corrupting the value of n.

Are my assumptions correct?

[rubenv]

When getStringForm() gets called, it expects that its n-parameter is a value that has already been modified by gettextPlurals().

I was implying that we should change this behavior, but since getStringForm() is external API, we can't right?

Either way: any value of n to my hypothetical getStringFormFor() would indeed need to be the unmodified n coming from the app.

[rubenswieringa]

So I suppose getStringForm() and getPlural() can't share a method that they both call then?

[rubenv]

Well, I don't think getStringForm() is used all that much. If getting good fallback language support comes at the expense of breaking getStringForm(): no problem, I'm willing to bump to a new version for this. The only main purpose of getStringForm() is to make getString() and getPlural() work: those are the ones that matter.

[rubenswieringa]

Alright, so do you want to completely get rid of getStringForm()?

[rubenv]

We can always add it back if people complain, but given the way fallback support works, it probably doesn't even make sense to have a method that behaves like the current getStringForm().

It'll be slightly annoying for the few people that depend on it, but we'll be saving them from pluralization bugs in Portuguese :-)

[rubenswieringa]

New PR: #250

[rubenv]

Cool, let's close this one.