Fix pluralization in default domain for non-germanic languages #2859
Conversation
MatthewWilkes
force-pushed
the
i18n_plurals_in_default_domain
branch
from
2cedc94
to
ba8362f
Compare
| # It doesn't override the previously loaded rule | ||
| self.assertEqual(inst.plural(0), 0) | ||
| self.assertEqual(inst.plural(1), 0) | ||
| self.assertEqual(inst.plural(2), 1) |
There was a problem hiding this comment.
I don't have a lot of translation experience but is this really correct? So the system doesn't choose the correct plural rules based on the selected language?
There was a problem hiding this comment.
This is all in the context of a single language.
The current behaviour is that the first plural wins, which for the default domain is the default pluraliser.
This change makes it so that for the default domain the second wins, which is the first user defined one.
There is an existing assumption that plurals within a language and domain pair are consistent which this patch doesn't affect
There was a problem hiding this comment.
Thank you, I missed that very important point. Is there anyone you want to review this or are you pretty confident things are good to go? I'm happy to merge it if you tell me to.
There was a problem hiding this comment.
I am happy with it personally, but I know that @tseaver had some concerns about the previous PR's approach, so maybe it would be good to have him confirm that this is sufficiently different?
|
FWIW I might have defined a |
|
You mean like #2102 last year? |
|
Hmm, yes apparently... why did we abandon that pr again? :-) Is the main issue that the previous PR was doing things for all domains and this one focuses on the default domain? Sorry I'm trying to help review this but maybe just causing more problems. |
|
I also didn't really understand it previously, but do more so now. I'm happy for you to go back to that PR and encourage a contribution now we have worked it out, if that is better. |
|
The fundamental difference between this PR and the other one is that this only changes the plural when adding to the It appears as if this is done because the default domain is considered a fallback for all other domains and in general if a translation occurs on the non-default domain it's expected it comes with an appropriate plural function whereas the default domain currently always has the default plural function. So if someone is adding a new translation to the default domain, we should pull the plural function from that translation. Please correct me if I'm wrong but with that in mind I prefer your location for the patch, as it's only done once at the top instead of twice. However I prefer the other patch for its approach of using only a single variable. Could you push that particular change and I'm happy to merge. |
MatthewWilkes
force-pushed
the
i18n_plurals_in_default_domain
branch
from
ba8362f
to
bff312d
Compare
|
@mmerickel I believe that's accurate. I have pushed the change to using the lambda function identity to check for overrides. |
|
@mmerickel Feel free. |
|
Thank you @MatthewWilkes and @quantum-omega! |
backport #2859 to 1.7-branch
This is a reimplementation of PR #2102 which is less invasive and tracks the bug to its original location.
When loading a new
.mofile, Pyramid checks if the domain already exists, and if so it merges the new catalog into the old one. It does not update thepluralfunction when this happens, as it's assumed that it is consistent inside a domain.The default domain is a special case, as it contains the other domains in a dictionary, so it is created ahead of time with an empty catalog, and the default pluralisation rule from #235. As all other files in the
messagesdomain will be merged to this catalog, the plural rule is never overwritten.This change introduces a flag to state if the default plural from #235 has been overridden. When merging two translations it checks if the domain is the default domain, and if the flag isn't set it replaces the
plurallambda expression with the gettext generated one and sets the flag.