Pluralization

[KL-7]

I tried to lay the ground for pluralization formatter. It's just a beginning and not yet ready for merging. This PR is more for discussion as I already have a couple of questions:

1. Currently number and the noun in the correct pluralization form are simply joined, but I'm going to make format method accepting real patterns where the number will replace some placeholder so the words order and phrasing can be adjusted by passing a language-specific patterns hash. After that is done instead of calling

   f.format(1, :one => 'ball')
   

   one would do smth like

   f.format(1, :one => '{0} ball')
   

   I assume that for this purpose existing tokenizers can be used, but I need to dig into that a bit more. What I want to know now is whether the overall idea is right.

2. Is raising an exception when required pattern is not found is a good idea? I'm asking because I don't see much exceptions across the project. Should some default value or nil be returned instead in that case?

3. Interface with format method accepting a number and a hash of patterns is a good start, but if someone need to format phrases with the same word (and therefore the same patterns hash) over and over it might be annoying to pass this hash every time or to store it somewhere outside of formatter object.

   If, e.g., someone needs to format different amounts of hours across the project it might be easier to setup formatter with the proper patterns in constructor and then pass to format method only a number. Drawback of this solution is that it requires one formatter for every word that needs to be formatted and it doesn't sound good if someone wants to pluralize a lot of different words simultaneously.

   What if we provide some interface for setting up formatter with a specific locale and a dictionary of words for every one of which user provides a patterns hash? Then he can use it like f.format(1, 'hour') and the formatter will find proper set of patterns for this word, pluralization rules for the number, choose pattern for this rule, and format the final phrase.

   May be the interface won't be exactly like that but I'd be happy to hear your thoughts on that.

[camertron]

Hey @KL-7, thanks for taking this on!

My initial concept for this feature was to override the % operator for string interpolation so you could do something like this:

replacements = { :horse_count => 3,
                 :horses => { :one => "is 1 horse",
                              :other => "are %{horse_count} horses" } }

"there %{horse_count:horses} in the barn" % replacements

Here's an alternate way to do it:

replacements = { :horse_count => 3,
                 :horses => { :one => "1 horse",
                              :other => "%{horse_count} horses" },
                 :to_be  => { :one => "is",
                              :other => "are" } }

"there %{horse_count:to_be} %{horse_count:horses} in the barn" % replacements

Just like we're already doing with most of TwitterCLDR's functionality, we should support this native, Ruby-ish way as well as provide a formatter like you've already described that the % function delegates to:

f = TwitterCldr::Formatters::Plurals::PluralFormatter.new("there %{horse_count:horses} in the barn", :es)
f.to_s(replacements)  # or f.format(replacements)

1. I don't think we'll need to make use of the existing tokenizers if we use normal Ruby interpolation syntax everywhere, i.e. "string %{inside} another" % { :inside => "blah" }
2. When you say "when the pattern isn't found", do you mean when a necessary plural form hasn't been given? In other words, are you asking if TwitterCLDR should throw an error if :other is missing from the hash in the example above and :horse_count is greater than 1?
3. If to_s or format accepts a full string and a hash of parameters that contain both the counts and the plural definitions (as I have shown above), there shouldn't be any need to create separate formatters for each word. Also, I like your idea of providing a basic list of plurals for each language, but I'm not sure where we're going to get that data - perhaps from the CLDR's abbreviation lists?

One last suggestion: What would you think about also accepting a Proc for each plural definition? That would allow for even greater flexibility:

replacements = { :horse_count => 3,
                 :horses => { :one => "is 1 horse",
                              :other => lambda { |context| context[:horse_count] <= 3 ? "are many beautiful horses" : "are many horses" } } }

"there %{horse_count:horses} in the barn" % replacements

[camertron]

Ah, I see what you mean. No, I don't think an error should be thrown here. Instead I think we should leave the original text in the string, so instead of "5 houses" you get "%{houses_count:houses}" without any replacements.

However! I've been considering for a while whether to include a global option to raise errors instead of just letting things slide. It really depends on the use case. At Twitter, we would most likely not want TwitterCLDR to raise errors because we occasionally launch features that aren't 100% translated anyway, but other projects (or other companies) might feel differently.

[KL-7]

Totally agree on that: it depends on the project. I can easily imagine people that won't be very happy if some untranslated text will get unnoticed into production because of some mistake or typo that was silently skipped by the formatter.

[KL-7]

@camertron, thanks for code review and such a detailed response. Can we formalize a bit the pluralization mechanism to make sure I clearly understand your vision? I'll explain how I see it and you let me know if I get smth wrong.

When we receive some string for processing we do the following:

1. treat interpolation pattern like %{horse_count:horses} as %{number_key:patterns_key};
2. fetch the number as a value at number_key in the interpolation hash;
3. fetch patterns hash as a value at patterns_key in the interpolation hash;
4. figure out pluralization rule (:one, :each, etc.) for the number;
5. knowing pluralization rule, retrieve the proper pattern from the patterns hash;
6. substitute the number for the number_key placeholders in the pattern;
7. substitute the result for %{number_key:patterns_key} in the string.

As you said throwing exception by default is not desirable, If during the process some required element is not found in the interpolation hash we ignore current interpolation pattern and move to the next one.

I definitely like the idea of accepting lambdas as it brings more flexibility, but I think it's not essential for the initial implementation and can be added later.

Does that sound good to you?

[camertron]

@KL-7 yes, that looks great. As a side note that may be helpful, the additional % functionality for strings is provided by the gettext gem: https://github.com/mutoh/gettext/blob/master/lib/gettext/core_ext/string.rb and isn't a Ruby thing as I assumed it was before. I think it will be important to preserve the existing gettext functionality while extending it with our special plural syntax, since that's what we use at Twitter and is widely used in the Ruby community as well.

[KL-7]

@camertron, right, interpolation like "%{foo}" % { :foo => 1 } is part of Ruby 1.9, but not of 1.8. That makes some problems because we can't easily call original String#% before (or after) our formatter as 1.8 throws ArgumentError when it come across %{ in the string. Should we include gettext as a dependency and delegate the rest of the work (if there is some other interpolations, besides pluralization, in the same string) to its String#% that works both on 1.8 and 1.9? Another question is whether we call super method before or after processing the string with our formatter? I don't see much difference at the moment, but may be there is some.

[camertron]

Oh man, bitten once again by the differences between 1.8 and 1.9! Fortunately we can get around the issue. At the moment, the magic method localize that you can call on a Date, Symbol, String, and Time, returns an instance of a subclass of LocalizedObject. So in the case of String, it returns an instance of LocalizedString, which means we have total control over formatting - we can create whatever methods we want, including a custom % that can delegate to base_obj's % method when necessary.

As a side note, it would be great if, for these localized objects, we could inherit from the original object so callers can perform all the same operations they can on the original object. That's a bit difficult because Ruby doesn't support multiple inheritance, but we should be able to accomplish the same thing by turning LocalizedObject into a mixin and including it in each localized subclass. Let me know what you think.

[KL-7]

@camertron, I'm afraid I don't understand how that applies to interpolation with String#%. If we want % operator to support pluralization we mentioned above we need to override it somehow. For that purpose I was going to implement PluralFormatter#format method that accepts a string to be interpolated and replacements hash. Then we can include into the core String class some module that defines % method. This method roughly does PluralFormatter.new(:en).format(self, replacements). But as we don't won't to break regular interpolation with String#% we need to call its original implementation somewhere in our version. And the problem arises with 1.8 as our string might contain %{ that is illegal sequence for interpolation string in this version of Ruby.

I think the best solution would be to include gettext that will give us same behavior of String#% in both rubies and after our work is done we can easily delegate the rest of interpolation to it simply by calling super. Another option is to borrow String#% implementation directly from gettext and add our pluralization on top of it.

Unfortunately, I don't see a good place for LocalizedString in this flow as long as we already overriding % directly in String class. As far as I can see we don't need anything more at the moment, so why do we need that wrapper?

[camertron]

I'd like to try to avoid monkey patching % in string if at all possible. I'm thinking specifically of some trouble I ran into a few months ago working inside the twitter.com codebase (internally called "the Monorail"). The % function was monkey patched in no less than four different places, once by gettext, then again by an i18n initializer and finally by two other 3rd-party gems. I don't think anybody had ever realized what was happening because most of the time the patches did roughly the same thing. I however was trying to modify how % behaved in a rather drastic way, and it was impossible to know which library to change or if my changes would play nicely with the other libraries. To make matters worse, there was no way to determine in which order the libraries would be required, meaning I would patch one instance of % to do what I wanted, and it would get replaced by another library. I would then try applying my changes to the other library, only to discover that the previous one was now being loaded last. It was a nightmare. Granted, it's probably fairly common in a large project to encounter dependency clashes like this, but it taught me how popular the % function is - everybody wants to monkey patch the crap out of it.

LocalizedString comes into play when users call the localize method on a string. This way, we only change the behavior of the classes we have control over:

"There %{horse_count:horses} in the barn".localize % { :horse_count => 3 ... }

This still doesn't explain how we ourselves are going to do %{} replacements however. I think we can simply add a utility function that will do standard interpolation as it's done in 1.9 and gettext, and perhaps even copy the gettext code over into twitter_cldr. In order to get regular 1.8 (and 1.9) number formatting as well, (eg. %2.f) we can use the existing % function which will have remained intact.

To recap, here's my opinion:

1. Add a utility function to TwitterCldr called interpolate that handles the most basic string substitution case, eg. "my %{variable}" % { :variable => "hello" }. We may be able to copy this from gettext or even do something as naïve as gsub.
2. Add the % function to LocalizedString that delegates to TwitterCldr::Shared::Plurals::PluralFormatter.
3. Write TwitterCldr::Shared::Plurals::PluralFormatter to figure out the right plural rule and use TwitterCldr.interpolate for the actual replacements.
4. Call % on the final string with a modified hash of interpolation options to handle the extra functionality that String#% provides (i.e. numbers).

What do you think?

[KL-7]

I felt that 'overriding String#%' won't be that easy =) You're right, monkey-patching is great, but sometimes it gets messy. I mostly agree with your plan, but have a couple of comments:

1. Implementation of String#% from gettext does all we need and I believe everything that the same method in 1.9 does ([simple interpolation with a hash](https://github.com/mutoh/gettext/blob/master/lib/gettext/core_ext/string.rb#L73, [formatted interpolation with a hash]%28https://github.com/mutoh/gettext/blob/master/lib/gettext/core_ext/string.rb#L66%29, [delegating to the core implementation]%28https://github.com/mutoh/gettext/blob/master/lib/gettext/core_ext/string.rb#L73%29) and I like most of it so with a couple of changes I can transform it into a utility function to use in this gem.
2. Looks like there's no LocalizedString class in the project at the moment and I still would prefer not to add it. Maybe it can be useful in the future when more features will be added, but at the moment monkey-patching String class (I doubt we can get away without monkey-patching at all) with localize method just to call a single % method on the returned LocalizedString instance doesn't sound like a good idea to me. At least not for a regular use case when you need to pluralize a number of string literals in your application. What if we monkey-patch String directly with some method that accepts a hash and calls PluralFormatter#format right away? Unfortunately, pluralize might be not the best name for this method but I think we can come up with some other good name for it.

[KL-7]

@camertron, now that things turned that way I start wondering why do we need to care about other kinds of interpolation at all? If we don't override String#% we can leave it for user to handle all other kinds of interpolation and care only about pluralization. If we put that in some new method of String class user can simply call this method with a pluralizations hash and then call % on it or do whatever he wants. What do you think?

[KL-7]

@camertron, I updated PluralFormatter implementation and specs. Check it out, please.

[camertron]

@KL-7, your points are well taken, here are a few additional things to think about:

1. I think we should use LocalizedString because it's consistent with the model we already have in place. You say DateTime.now.localize.to_s, not DateTime.now.format_datetime(blah). In addition, it would be difficult to change or remove a function like pluralize out from under our users. The localize function is already embedded in the functionality of the gem, so it's likely that will always exist. The additional memory required to create instances of LocalizedString should be minimal - in this case, consistency should win.
2. I initially wanted to support all three types of interpolation (plurals, normal, and numbers) because it's more straightforward for our users. Instead of doing something like this:

"there %{horse_count:horses} in the barn, %{user}!".pluralize(:horse_count => count,
                                                              :horses => "are %{horse_count}") % { :user => current_user }

you can combine the hashes and do this instead:

"there %{horse_count:horses} in the barn, %{user}!".pluralize(:horse_count => count,
                                                              :horses => "are %{horse_count}",
                                                              :user => current_user)

We aren't obligated to provide this functionality, but it's certainly nice, and wouldn't take much effort to implement. Besides, we're going to need an interpolation function anyway - why not provide it to everyone? I'm still on the fence with numbers, but if we support everything else, we might as well do them too. After all, it's just a single call to String#%.

1. If we borrow gettext's % function, we'll need to copy over the license file as per the license's restrictions (I think it's FreeBSD).

[camertron]

Better to accept a hash of options here and use TwitterCldr.get_locale if no locale is specified.

[KL-7]

I see most of the formatters use Formatters::Base#extract_locale and TwitterCldr.get_locale (that also takes FastGettext locale into consideration) is mostly used in localize methods. Is that on purpose? Would it be more consistent to use extract_locale here as in other formatters?

[camertron]

Ah, right you are. extract_locale is better.

[camertron]

This is looking awesome! Just a few more changes and I'll merge it in. Looks like your most recent commits don't handle the regular interpolation case for "I %{verb} watermelons" % { :verb => "love" } Also, you haven't added support for LocalizedString. I know we disagreed a bit on that, but I really think we should add it. Should I handle it or would you like to do it?

[KL-7]

When I add interpolation utility function from gettext this line will be replaced with a call to this function.

[KL-7]

@camertron, ah, sorry, I forgot to mention that your last arguments did convince me =) I'm going to extract interpolation function from gettext and use it for handling everything that left after our pluralization process. And then I'll add LocalizedString class - consistency is important thing.

Btw, gettext offers Ruby license or LGPL. Licensing is not my strongest skill, but I can read agreements and learn what is expected from us if we're going to take some code from this project.

[KL-7]

@camertron, two more things. Is it better to delegate to that general interpolation function right inside PluralFormatter#format or in LocalizedString#% that will first call formatter and then pass the result to this utility function? And I can't figure out whether there is any difference in making pluralization first and then regular interpolation or vice versa. What do you think?

[camertron]

@KL-7 Ok cool, glad we agree! I sincerely appreciate having these thoughtful discussions. OSS licenses generally let anyone use the code, provided you include a copy of the license in your derived work. We can include a copy of the LGPL license in our LICENSE file and specify what parts of the gem it applies to.

I don't think there's any difference in calling the general interpolation function before or after PluralFormatter#format - you're free to decide what's best!

[KL-7]

Hey @camertron, I added LocalizedString class and I'm not sure that I tested it properly. When a hash is passed into it, it delegates everything to the PluralFormatter and to test that behavior I simply copied tests from the test suit of PluralFormatter. Is that the right way to go?

Another question arose related once again to the TwitterCldr#interpolate method I'm going to add. We already agreed that our pluralization formatter won't raise any exceptions if some pluralization rule or pattern is missing (because there are cases when you don't have translation at the moment but still want to get at least something in the result string). On the other hand regular String#% from Ruby 1.9 throws KeyErorr when you don't provide a value for interpolation:

1.9.3-p125 :001 > '%{name}' % {}
KeyError: key{name} not found

Should we mimic that behavior as it's done, e.g., in i18n gem, or should we silently ignore that situation and leave the string unchanged?

[camertron]

Hmm that's a tough question. In this case, I think we should throw an error to maintain consistency with what's already in place. Anyone who's using this type of interpolation already in their code should be expecting KeyError to be thrown, and may even have specific rescue blocks to catch it. It would be nice to provide the same functionality. What do you think?

[KL-7]

@camertron, I agree with you, but it's slightly inconsistent with the way we're treating missing translations, though, as it's a bit different situation, throwing an exception here and not throwing it for missing pluralization rules does make sense to me.

What do you think about tests for LocalizedString#%? I'm seriously puzzled with testing methods like that. From one point of view we want to test its behavior as a black-box, so all these tests make sense, but from the other when the hash is passed this method delegates everything else to the PluralFormatter#format method that is already thoroughly tested in a different place and it feels like there is no need in testing the work it's doing again here.

[camertron]

Yes, you're right, there's no need to test all the functionality LocalizedString delegates to PluralFormatter#format. There really only needs to be a single test: make sure that calling localize on a string returns an instance of LocalizedString. It doesn't look like LocalizedString needs any specific tests of its own. I've generally combined the specs for patching core Ruby objects with their corresponding localized object wrapper. Time is a good example.

[KL-7]

@camertron, I added interpolation function and updated LocalizedString#% to use it. And there are some more things I'd like to discuss with you:

1. I borrowed some tests and parts of the implementation from i18n and gettext gems, but didn't add any notice about that yet. Should I add some references directly into the interpolation.rb file and then licenses texts into NOTICE file?

2. I reduced amount of specs for LocalizedString#%, but decided to leave a number of them as integration tests to make sure pluralization and interpolation play nice together. Btw, that helped me to find an issue in the regexp for pluralization placeholders. Does that look ok? Should I add smth to it?

3. The way we're handling missing information still looks a bit confusing to me, but seems like it works as we discussed: ignores any missing pluralization stuff, but throws exceptions when something is missing for regular interpolation.

4. Not sure whether it's intentional, but I'd say the gem lacks comments very much. If the interface of this feature is settled, should I add some comments to my code?

5. Not related to this feature, but to the gem in general. What do you think if I bring its directory structure (specifically lib directory) to a common form that'd look smth like that:

   $ tree -d twitter_cldr
   |-- lib/
   |   |-- twitter_cldr.rb
   |   `-- twitter_cldr/
   |       |-- core_ext/
   |       |-- formatters/
   |       |-- shared/
   |       |-- tokenizers/
   |       |-- interpolation.rb
   |       `-- version.rb
   |-- spec/
   |-- Gemfile
   |-- Rakefile
   `-- twitter_cldr.gemspec
   

[camertron]

This file should probably not be hanging out right inside lib - consider moving it into a child directory.

[KL-7]

If I restructure directories a bit as I suggested here this file will live under lib/twitter_cldr.

[camertron]

Hey @KL-7 just one comment on your code, but otherwise it's looking darn good.

1. Yes, please add a comment to interpolate.rb and interpolate_spec.rb specifying where the code came from originally. Add the additional license info into LICENSE and add a comment above saying why you included it (something like "Parts of this gem adapted from the gettext gem, available at [url]. The source code requires that the license below accompany it whenever it is copied.")
2. Looks great. The integration tests you've added are perfect.
3. I didn't realize you'd have to define KeyError, that's an interesting conundrum. I still think that's the right behavior however, seeing as how we are in effect porting 1.9 functionality over to 1.8.
4. I've found that the Ruby community very much believes in "self-documenting" code. When I first heard that term, I thought it was total crap. I mean, how can you expect someone to know what you're code is doing if you don't write good comments? Anyone who writes Java for a living or studied computer science in college for example will tell you to comment the heck out of everything. In Rubyland, it seems much more customary to only comment the weird things, like complicated regexes and tricky bits of recursion. Ruby programmers also focus on writing readable, expressive tests that are supposed to serve partly as comments on how the code was designed to be used. With that in mind, you are free to write as many comments as you like. Try to focus on the regexes you have, especially those parts where you have like 4 && calls in a row. Otherwise, let your tests do the talking :)
5. Sure, I'd be in favor of reorganizing the gem a bit to follow that outline. No reason not to, I suppose.

And finally, to the other topic at hand. When my coworkers and I initially talked about what pluralization implementation would be best for TwitterCLDR, we agreed that the implementation you have created was the right answer. Just yesterday, we had another discussion that has augmented the implementation a bit. The good news is we can keep all of your changes, and simply offer the augmentations as an additional way to write plurals. Here's an example of this second way:

'there %{horse_count("one": "is one horse", "other": "are %{horse_count} horses")} in the barn' % { :horse_count => 3 }

This technique unifies the whole sentence together, meaning the translators of this phrase don't have to translate first the whole sentence, then each individual plural rule, which might be confusing. Imagine if you were asked to translate just the string "is one horse" without any context at all. It simply doesn't make sense without the whole sentence. In other languages like Japanese, you might even want to put the plural in an entirely different place. Finally, it's easier on the programmer who won't have to build a hash with the correct options for the current language.

In some projects, however, it might be easier for the programmer to specify a hash. Imagine, for example, a project that isn't translated. The programmer would have no objections to supplying a hash with :one and :other rules because they will never change, and s/he can see the phrases in context in the code.

Lastly, notice how I've used JSON to represent the plural data. I wanted to make it as easy as possible to parse using a standard format. It still might be tricky, and we can definitely talk about it. What are your thoughts?

[KL-7]

@camertron, I added license information and some notes regarding the code adapted from i18n and gettext gems. Please, check it out and let me know if there's smth I should change.

There are some comment from me:

1. I agree with the idea that current implementation might be not very translator-friendly. On the other hand I don't like very much the format you suggested. First, it's not very clear why horse_count looks like function name in it. Second, it will require some hand-made parsing, gsub-ing or whatever that I'd prefer to avoid. What do you think about a bit modified version like that:

   'there %{{ "horse_count": { "one": "is one horse", "other": "are %{horse_count} horses" } }} in the barn'

   The advantage of this format is that inside %{...} we have valid JSON hash that can be easily parsed into a Ruby hash using JSON.parse. I'm not sure about doubling outside braces, but the intention is to make it easier to find this pattern in a string using regexp. Though, while %{{ in the beginning allows us to distinguish that kind of interpolation from a regular one, the closing }}} might cause ambiguity. How about some other combination like %[{...}] or %<{...}>?

   Anyway, do you want this addition to be a part of this PR? If not, I can make any necessary final fixes, add some comments to public method, clean up commits history, and rebase it on master. Then you'll be able to merge it in and the new syntax will go in a different pull request.

2. I'd be happy to change directories structure right away, but with this big PR and opened PRs from timothyandrew, I'm afraid we might get in a difficult situation while trying to merge or rebase them after the directories are moved/renamed. I tried it localy and looks like it's too cool even for git. Do you have idea how we can handle that with the smallest possible complications?

[camertron]

@KL-7 NOTICE looks good, thanks for adding the additional licensing text.

1. I completely agree with you, horse_count does look like a function call and for no good reason. The JSON idea is really nice, I love the simplicity of it - easy to understand for the programmer, translator, and easy-ish for us to implement. In terms of which delimiter to use instead of {} it makes sense to use something else like [] or <> as you've suggested to avoid regexp complications. However, I would hesitate to use <> because it looks a little too much like ERB, and I might also avoid [] because we may at some point want to support JSON arrays. What would you think about using pipe characters || or parentheses () instead? I don't think those would clash with any of the other syntax. Here's how it would look with pipes:

'there %|{ "horse_count": { "one": "is one horse", "other": "are %{horse_count} horses" } }| in the barn'

Finally, we should make this a separate pull request instead of trying to fit too much into this one. Go ahead and put whatever polish you'd like on this PR and I'll happily merge it in. Also, do you really need to rebase? Might be nice to keep your commit history intact.
2. Hmm yes, perhaps it would be a good idea to wait until things quiet down before rearranging the directory structure. The only way I can think to do it with the smallest number of complications would be to lock everything down and not accept any new feature/pull requests for a while. We could then perform the rearrangement and converse individually with others about conforming to the new structure. Another strategy would to simply ask everyone to reorganize their local copies first, then merge or rebase master. We're lucky because we only have 2 or 3 people that would have to do this - imagine trying to rearrange files in a project like Twitter Bootstrap...

[KL-7]

1. Glad you like the idea, but if you ask me %|{...}| looks really strange. So I still think %<{...}> is the best choice. It does look a bit like ERB, but 1.9 have %<foo>d syntax for formatted named placeholders in interpolation anyway. Though, looks like %({...}) might work as well.

   If I remember correctly you can't merge in a PR that doesn't cleanly apply to the current master branch, or can you? I though if I have to rebase it I can squash some fix commits and the initial implementation (that went into the wrong direction) at the same time. But if you think there's no need in it I won't touch anything.

2. I think there's no need to rush, so we can wait at least until three big currently opened PR get merged in.

[camertron]

1. That's fine, we can use <> if you like that better. It's also fine with me if you want to rebase, but I don't have any objections to merging manually using the terminal instead of via Github. I'll leave it up to you - if you thing rebasing will be cleaner, then that's totally cool.
2. Sounds good. I'm thinking of a) plurals and b) normalization - are there any others we should wait for?

[KL-7]

@camertron, I rebased pluralization branch against master, cleaned up commits history a bit and added comments for interpolation and pluralization methods. I'm pretty much satisfied with this PR now. If you feel the same, you can merge it.