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

Fix 220: Create plural module #241

Closed
wants to merge 1 commit into from
Closed

Fix 220: Create plural module #241

wants to merge 1 commit into from

Conversation

rxaviers
Copy link
Member

Borrow santhoshtr/CLDRPluralRuleParser.

Ref #220

  • Implementation (basically a wrapper);
  • Unit tests;
  • Build;
  • Functional tests;
  • Docs;

// 2: UMD outro
return contents
.replace( /\(function\(root, factory\)[\s\S]*?}\(this, function\(\) {/, "var CLDRPluralRuleParser = (function() {" ) /* 1 */
.replace( /}\)\);\s+$/, "}());" ); /* 2 */
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the second line comment above this line? Same for the other. Also make it more explicit, e.g. "Remove UMD outro"

@rxaviers rxaviers self-assigned this May 19, 2014
@rxaviers rxaviers removed their assignment May 20, 2014
@rxaviers rxaviers force-pushed the plural branch 3 times, most recently from 971866e to ce4261c Compare August 20, 2014 21:29
@rxaviers
Copy link
Member Author

Ready for review (or to be landed).

A good summary of the changes made by this PR can be seen in the README diff. It creates one more module called globalize/plural.js. This new module depends on Globalize core globalize.js (and cldr.js) and nothing else (note that santhoshtr/CLDRPluralRuleParser is embedded totaling 1.9kb minified+gziped). Prior to using the pural module, one needs to load cldr/supplemental/plurals.json data. It implements both .formatPlural( value, messageData [, formatValue ] ) (more information) and .plural( value ) (more information) methods.

Globalize.formatPlural( 2, messageData ); // "You have 2 unread messages"

Globalize.formatPlural( 12345, messageData, Globalize.formatNumber( 12345 ) );
// 'You have 12,345 unread messages'
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to use these "output comments" consistently. Either put them after the line in question, before, or on the same line, but don't mix it, like here and with the three above.

Same for the other method.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeap. done.

zero: "zero"
};

assert.equal( en.formatPlural( 0, fakeFormatValue ), "other", "" );
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the empty message argument, its optional. The API docs for wrong until a week ago.

Borrow santhoshtr/CLDRPluralRuleParser.

Ref #220
@rxaviers
Copy link
Member Author

Thanks @jzaefferer, I've updated it based on your comments.

@jzaefferer
Copy link
Contributor

This should provide a decent error message:

Globalize.formatPlural(2, { one: "{0} thing" })

Instead I get TypeError: Cannot read property 'replace' of undefined. It should tell me which plural form it used.

This uses the value instead of the empty string:

// "2 things"
Globalize.formatPlural(2, { other: "{0} things" }, "")

@rxaviers
Copy link
Member Author

rxaviers commented Sep 1, 2014

Thanks @jzaefferer, both fixed and covered by new functional tests.

@rxaviers rxaviers closed this in 5332451 Sep 1, 2014
@rxaviers rxaviers deleted the plural branch September 9, 2014 12:40
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.

2 participants