Add unit formatting module #512

Closed
wants to merge 20 commits into
from

Conversation

Projects
None yet
4 participants
@alunny
Collaborator

alunny commented Sep 24, 2015

This is picking up @rxaviers's patch in #254 to solve #252.

rxaviers and others added some commits May 20, 2014

Unit: Create it
updated to merge cleanly
@alunny

This comment has been minimized.

Show comment
Hide comment
@alunny

alunny Sep 25, 2015

Collaborator

I think I can make it work by heavily cargo-culting relative-timestamp - new patch forthcoming.

Collaborator

alunny commented Sep 25, 2015

I think I can make it work by heavily cargo-culting relative-timestamp - new patch forthcoming.

@alunny

This comment has been minimized.

Show comment
Hide comment
@alunny

alunny Sep 25, 2015

Collaborator

(I have spaces instead of tabs in many places - will update that when this is good to merge otherwise)

Collaborator

alunny commented Sep 25, 2015

(I have spaces instead of tabs in many places - will update that when this is good to merge otherwise)

src/unit.js
+ *
+ * Format units such as seconds, minutes, days, weeks, etc.
+ */
+Globalize.prototype.formatUnit = function( value, unit, options ) {

This comment has been minimized.

@rxaviers

rxaviers Sep 25, 2015

Member

Let's update this according to the below please. So, it's accessible via both static function (Globalize.formatUnit) and instance method (new Globalize(locale).formatUnit)

Globalize.formatUnit =
Globalize.prototype.formatUnit = function( ...
@rxaviers

rxaviers Sep 25, 2015

Member

Let's update this according to the below please. So, it's accessible via both static function (Globalize.formatUnit) and instance method (new Globalize(locale).formatUnit)

Globalize.formatUnit =
Globalize.prototype.formatUnit = function( ...

This comment has been minimized.

@alunny

alunny Sep 28, 2015

Collaborator

fixed on my local branch (commenting for my own tracking benefit)

@alunny

alunny Sep 28, 2015

Collaborator

fixed on my local branch (commenting for my own tracking benefit)

src/unit.js
+ var pluralGenerator;
+
+ if ( typeof value !== "number" ) {
+ throw new Error( "Value is not a number" );

This comment has been minimized.

@rxaviers

rxaviers Sep 25, 2015

Member

Let's reuse these data type validator https://github.com/jquery/globalize/blob/master/src/number.js#L151 (validateParameterTypeNumber).

@rxaviers

rxaviers Sep 25, 2015

Member

Let's reuse these data type validator https://github.com/jquery/globalize/blob/master/src/number.js#L151 (validateParameterTypeNumber).

This comment has been minimized.

@alunny

alunny Sep 28, 2015

Collaborator

fixed on local branch

@alunny

alunny Sep 28, 2015

Collaborator

fixed on local branch

src/unit.js
+ }
+
+ if ( !unit ) {
+ throw new Error( "Missing unit" );

This comment has been minimized.

@rxaviers

rxaviers Sep 25, 2015

Member

Let's reuse these data type validator https://github.com/jquery/globalize/blob/master/src/number.js#L150 (validateParameterPresence).

@rxaviers

rxaviers Sep 25, 2015

Member

Let's reuse these data type validator https://github.com/jquery/globalize/blob/master/src/number.js#L150 (validateParameterPresence).

This comment has been minimized.

@alunny

alunny Sep 28, 2015

Collaborator

fixed on local branch

@alunny

alunny Sep 28, 2015

Collaborator

fixed on local branch

src/unit.js
+ pluralGenerator = this.pluralGenerator();
+
+ return unitFormat( value, unit, options, pluralGenerator, this.cldr, this );
+};

This comment has been minimized.

@rxaviers

rxaviers Sep 25, 2015

Member

In general, .formatUnit() should be an alias for .unitFormatter(). Like .formatNumber() https://github.com/jquery/globalize/blob/master/src/number.js#L148-L154 is for .numberFormatter() https://github.com/jquery/globalize/blob/master/src/number.js#L107-L137

Note the process of setting up the formatter in general is somewhat an expensive operation. This process includes traversing CLDR data and cherry-picking the specific data (which I name properties) for the particular options used by the formatter. Therefore, the formatter generator should return a function that in turn allows to execute the formatting (i.e., allow user to setup once and execute several times for optimal performance; and to allow runtime compiled formatters). Please, just let me know on any questions.

@rxaviers

rxaviers Sep 25, 2015

Member

In general, .formatUnit() should be an alias for .unitFormatter(). Like .formatNumber() https://github.com/jquery/globalize/blob/master/src/number.js#L148-L154 is for .numberFormatter() https://github.com/jquery/globalize/blob/master/src/number.js#L107-L137

Note the process of setting up the formatter in general is somewhat an expensive operation. This process includes traversing CLDR data and cherry-picking the specific data (which I name properties) for the particular options used by the formatter. Therefore, the formatter generator should return a function that in turn allows to execute the formatting (i.e., allow user to setup once and execute several times for optimal performance; and to allow runtime compiled formatters). Please, just let me know on any questions.

This comment has been minimized.

@alunny

alunny Sep 28, 2015

Collaborator

I'm a little confused by this - is it right to say unitFormatter should be the only function that touches the cldr data, and that src/unit/format.js should operate on the already extracted content?

Here's an example of one approach I was looking at for this, let me know if it looks sound.
https://gist.github.com/alunny/c9777c4c171b986fad9f

@alunny

alunny Sep 28, 2015

Collaborator

I'm a little confused by this - is it right to say unitFormatter should be the only function that touches the cldr data, and that src/unit/format.js should operate on the already extracted content?

Here's an example of one approach I was looking at for this, let me know if it looks sound.
https://gist.github.com/alunny/c9777c4c171b986fad9f

This comment has been minimized.

@rxaviers

rxaviers Sep 29, 2015

Member

Your gist diff seems perfect!

The point of keeping src/unit/format.js CLDR-free (and as light as possible) is that it goes in the runtime module. The other functions that setup and prepare/create the properties (e.g., src/unit/get.js) can be safely ignored in the runtime modules.

@rxaviers

rxaviers Sep 29, 2015

Member

Your gist diff seems perfect!

The point of keeping src/unit/format.js CLDR-free (and as light as possible) is that it goes in the runtime module. The other functions that setup and prepare/create the properties (e.g., src/unit/get.js) can be safely ignored in the runtime modules.

This comment has been minimized.

@alunny

alunny Sep 29, 2015

Collaborator

Sounds good - had to update the tests too, but now everything's passing.

@alunny

alunny Sep 29, 2015

Collaborator

Sounds good - had to update the tests too, but now everything's passing.

src/unit/format.js
+ * Duration Unit (for composed time unit durations) is not implemented.
+ * http://www.unicode.org/reports/tr35/tr35-35/tr35-general.html#durationUnit
+ */
+return function( value, unit, options, pluralGenerator, cldr, globalize ) {

This comment has been minimized.

@rxaviers

rxaviers Sep 25, 2015

Member

Let's remove globalize as an argument. I know this comes from my previous branch. But, instead we should pass the plural formatter like https://github.com/jquery/globalize/blob/master/src/relative-time.js#L68. Because, it allows the runtime compiler to appropriately use a precompiled plural generator here.

@rxaviers

rxaviers Sep 25, 2015

Member

Let's remove globalize as an argument. I know this comes from my previous branch. But, instead we should pass the plural formatter like https://github.com/jquery/globalize/blob/master/src/relative-time.js#L68. Because, it allows the runtime compiler to appropriately use a precompiled plural generator here.

This comment has been minimized.

@rxaviers

rxaviers Sep 25, 2015

Member

Just noticed we're already passing pluralGenerator above. But, below we're using globalize instance. Let's get the below fixed (and make the calling function passes the correct arguments).

@rxaviers

rxaviers Sep 25, 2015

Member

Just noticed we're already passing pluralGenerator above. But, below we're using globalize instance. Let's get the below fixed (and make the calling function passes the correct arguments).

This comment has been minimized.

@alunny

alunny Sep 28, 2015

Collaborator

fixed in local branch

@alunny

alunny Sep 28, 2015

Collaborator

fixed in local branch

src/unit/format.js
+ // "There is a known problem with some languages in the long form in that
+ // the divisor should be inflected. This will probably require the future
+ // addition of a special 'divisor' form of units commonly used in the
+ // divisor." UTS#35

This comment has been minimized.

@rxaviers

rxaviers Sep 25, 2015

Member

We should ensure this text (and therefore, the logic we're using here) is still valid. http://www.unicode.org/reports/tr35/tr35-general.html#Unit_Elements

@rxaviers

rxaviers Sep 25, 2015

Member

We should ensure this text (and therefore, the logic we're using here) is still valid. http://www.unicode.org/reports/tr35/tr35-general.html#Unit_Elements

This comment has been minimized.

@alunny

alunny Sep 28, 2015

Collaborator

I think the compound unit logic is incorrect now - will fixup and write some tests.

@alunny

alunny Sep 28, 2015

Collaborator

I think the compound unit logic is incorrect now - will fixup and write some tests.

test/unit/unit/format.js
+ formatUnit( 2, "year", { form: "narrow" }, oneOrOtherPluralGenerator, cldr, globalize ),
+ "2y"
+ );
+});

This comment has been minimized.

@rxaviers

rxaviers Sep 25, 2015

Member

Let's simplify the whole assertions above by creating a custom assertion like https://github.com/jquery/globalize/blob/master/test/unit/date/format.js#L60-L71. This way we avoid boilerplate code.

@rxaviers

rxaviers Sep 25, 2015

Member

Let's simplify the whole assertions above by creating a custom assertion like https://github.com/jquery/globalize/blob/master/test/unit/date/format.js#L60-L71. This way we avoid boilerplate code.

This comment has been minimized.

@alunny

alunny Sep 28, 2015

Collaborator

fixed on local branch

@alunny

alunny Sep 28, 2015

Collaborator

fixed on local branch

src/unit.js
+return Globalize;
+
+});
+

This comment has been minimized.

@rxaviers

rxaviers Oct 1, 2015

Member

Space vs tabs indentation is still used here. Tabs should be used for indentation.

@rxaviers

rxaviers Oct 1, 2015

Member

Space vs tabs indentation is still used here. Tabs should be used for indentation.

This comment has been minimized.

@rxaviers

rxaviers Oct 1, 2015

Member

There's an extra empty line in the bottom of the file.

@rxaviers

rxaviers Oct 1, 2015

Member

There's an extra empty line in the bottom of the file.

This comment has been minimized.

@rxaviers

rxaviers Oct 1, 2015

Member

Wondering why grunt jscs:source didn't caught these...

@rxaviers

rxaviers Oct 1, 2015

Member

Wondering why grunt jscs:source didn't caught these...

src/unit.js
+
+ "./plural"
+], function( Globalize, validateParameterPresence, validateParameterTypeNumber,
+ validateParameterTypePlainObject, unitFormat, unitFormatterFn, unitGet ) {

This comment has been minimized.

@rxaviers

rxaviers Oct 1, 2015

Member

Unused unitFormat.

@rxaviers

rxaviers Oct 1, 2015

Member

Unused unitFormat.

src/unit/get.js
+ * @unit [String] The full type-unit name (eg. duration-second), or the short unit name
+ * (eg. second).
+ *
+ * FIXME

This comment has been minimized.

@rxaviers

rxaviers Oct 1, 2015

Member

Let's replace FIXME by adding a description for @form and @cldr attributes.

@rxaviers

rxaviers Oct 1, 2015

Member

Let's replace FIXME by adding a description for @form and @cldr attributes.

src/unit/get.js
+ ret = stripPluralGarbage( ret );
+
+ // Compound Unit, eg. "foot-per-second" or "foot/second".
+ if ( !ret && ( /-per-|\// ).test( unit ) ) {

This comment has been minimized.

@rxaviers

rxaviers Oct 1, 2015

Member

I like allowing the use of / as well as -per-. But, -per- is what CLDR has. So, in the beginning of this function we need unit = unit.replace( /\// , "-per-" );, otherwise kilometer/hour would not use the precomputed kilometer-per-hour form, but instead the recursively generated one. It would lead to different results than having used the -per- form.

@rxaviers

rxaviers Oct 1, 2015

Member

I like allowing the use of / as well as -per-. But, -per- is what CLDR has. So, in the beginning of this function we need unit = unit.replace( /\// , "-per-" );, otherwise kilometer/hour would not use the precomputed kilometer-per-hour form, but instead the recursively generated one. It would lead to different results than having used the -per- form.

src/unit/categories.js
+ * Return all unit categories.
+ */
+return [ "acceleration", "angle", "area", "duration", "length", "mass", "power", "pressure",
+"speed", "temperature", "volume" ];

This comment has been minimized.

@rxaviers

rxaviers Oct 1, 2015

Member

An alternative to this approach is generating this dynamically once and keep it cached for further uses. Although, this could remain untouched for now and get improved later.

@rxaviers

rxaviers Oct 1, 2015

Member

An alternative to this approach is generating this dynamically once and keep it cached for further uses. Although, this could remain untouched for now and get improved later.

This comment has been minimized.

@alunny

alunny Oct 8, 2015

Collaborator

Modified this slightly to add "digital" category. Are there other places in the codebase where you're caching once based on the cldr data?

@alunny

alunny Oct 8, 2015

Collaborator

Modified this slightly to add "digital" category. Are there other places in the codebase where you're caching once based on the cldr data?

test/unit/unit/format.js
+ assert.unitFormat( 1, "speed-mile-per-hour", {}, "1 mile per hour" );
+ assert.unitFormat( 100, "speed-mile-per-hour", {}, "100 miles per hour" );
+ assert.unitFormat( 1, "consumption-mile-per-gallon", {}, "1 mile per gallon" );
+ assert.unitFormat( 100, "consumption-mile-per-gallon", {}, "100 miles per gallon" );

This comment has been minimized.

@rxaviers

rxaviers Oct 1, 2015

Member

Let's insert a unit without category for completeness, e.g., mile-per-hour.

@rxaviers

rxaviers Oct 1, 2015

Member

Let's insert a unit without category for completeness, e.g., mile-per-hour.

This comment has been minimized.

@rxaviers

rxaviers Oct 1, 2015

Member

Also, let's include a compound without a precomputed value, e.g., megabyte-per-second.

@rxaviers

rxaviers Oct 1, 2015

Member

Also, let's include a compound without a precomputed value, e.g., megabyte-per-second.

+ assert.unitFormat( 1, "month", { form: "narrow" }, "1m" );
+ assert.unitFormat( 2, "month", { form: "narrow" }, "2m" );
+ assert.unitFormat( 1, "year", { form: "narrow" }, "1y" );
+ assert.unitFormat( 2, "year", { form: "narrow" }, "2y" );

This comment has been minimized.

@rxaviers

rxaviers Oct 1, 2015

Member

Let's insert a unit with its category for completeness, e.g., speed-mile.

@rxaviers

rxaviers Oct 1, 2015

Member

Let's insert a unit with its category for completeness, e.g., speed-mile.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Oct 1, 2015

Member

Great work so far!! I have left a couple of comments above. Other than that, I also miss these points:

  • Update docs (refs 1, 2, 3);
  • Build unit module (refs 1, 2);
  • Create functional tests (ref 1);
  • Build runtime module (refs 1, 2)

Just let me know on any questions and thanks for the great work!

Member

rxaviers commented Oct 1, 2015

Great work so far!! I have left a couple of comments above. Other than that, I also miss these points:

  • Update docs (refs 1, 2, 3);
  • Build unit module (refs 1, 2);
  • Create functional tests (ref 1);
  • Build runtime module (refs 1, 2)

Just let me know on any questions and thanks for the great work!

@alunny

This comment has been minimized.

Show comment
Hide comment
@alunny

alunny Oct 8, 2015

Collaborator

Thanks for the great feedback - working on cleaning up this branch now :)

Collaborator

alunny commented Oct 8, 2015

Thanks for the great feedback - working on cleaning up this branch now :)

test/unit/unit/format.js
+ formatUnit( value, unitProperties, oneOrOtherPluralGenerator, compoundUnitPattern ),
+ expected
+ );
+};

This comment has been minimized.

@alunny

alunny Oct 8, 2015

Collaborator

I'm not really happy with the unitFormat function - it's a bit too coupled with Globalize.formatUnit. Suggestions for refactoring are welcome.

@alunny

alunny Oct 8, 2015

Collaborator

I'm not really happy with the unitFormat function - it's a bit too coupled with Globalize.formatUnit. Suggestions for refactoring are welcome.

This comment has been minimized.

@rxaviers

rxaviers Oct 8, 2015

Member

Feel free to take or not this suggestion. But, I would remove the "long" default for form (in here, unit tests) and I would collapse both unit and compountUnitPattern in a single function unit/properties.js to avoid duplicating changes here and there.

@rxaviers

rxaviers Oct 8, 2015

Member

Feel free to take or not this suggestion. But, I would remove the "long" default for form (in here, unit tests) and I would collapse both unit and compountUnitPattern in a single function unit/properties.js to avoid duplicating changes here and there.

This comment has been minimized.

@alunny

alunny Oct 8, 2015

Collaborator

I think that's a good idea, thanks.

@alunny

alunny Oct 8, 2015

Collaborator

I think that's a good idea, thanks.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Oct 8, 2015

Member

An alternative to this approach is generating this dynamically once and keep it cached for further uses. Although, this could remain untouched for now and get improved later.

Modified this slightly to add "digital" category. Are there other places in the codebase where you're caching once based on the cldr data?

Ok and nope we have nothing similar in our codebase currently. Actually, I dislike to embed any sort of data in the code unless absolutely needed like in this case and a couple others: [1] and [2].

Member

rxaviers commented Oct 8, 2015

An alternative to this approach is generating this dynamically once and keep it cached for further uses. Although, this could remain untouched for now and get improved later.

Modified this slightly to add "digital" category. Are there other places in the codebase where you're caching once based on the cldr data?

Ok and nope we have nothing similar in our codebase currently. Actually, I dislike to embed any sort of data in the code unless absolutely needed like in this case and a couple others: [1] and [2].

src/unit.js
+ compoundUnitPattern = unitGet( "per", form, this.cldr ).compoundUnitPattern;
+ unitProperties = unitGet( unit, form, this.cldr );
+
+ return unitFormatterFn( unitProperties, this.pluralGenerator(), compoundUnitPattern );

This comment has been minimized.

@rxaviers

rxaviers Oct 8, 2015

Member

I'd like to suggest two changes:

  1. Simplify how you get compoundUnitPattern (I think reusing unitGet for that is an overkill and kinda confusing)
- compoundUnitPattern = unitGet( "per", form, this.cldr ).compoundUnitPattern
+ compoundUnitPattern: this.cldr.main(["units", form, "per/compoundUnitPattern"])
  1. Collapse properties:
-   var compoundUnitPattern, form, unitProperties;
+   var form, properties;

    validateParameterPresence( unit, "unit" );
    validateParameterTypePlainObject( options, "options" );

    form = options.form || "long";
-   compoundUnitPattern = unitGet( "per", form, this.cldr ).compoundUnitPattern;
-   unitProperties = unitGet( unit, form, this.cldr );
+   properties = {
+       compoundUnitPattern: this.cldr.main(["units", form, "per/compoundUnitPattern"])
+       unit: unitGet( unit, form, this.cldr )
+   }

-   return unitFormatterFn( unitProperties, this.pluralGenerator(), compoundUnitPattern );
+   return unitFormatterFn( properties, this.pluralGenerator() );
@rxaviers

rxaviers Oct 8, 2015

Member

I'd like to suggest two changes:

  1. Simplify how you get compoundUnitPattern (I think reusing unitGet for that is an overkill and kinda confusing)
- compoundUnitPattern = unitGet( "per", form, this.cldr ).compoundUnitPattern
+ compoundUnitPattern: this.cldr.main(["units", form, "per/compoundUnitPattern"])
  1. Collapse properties:
-   var compoundUnitPattern, form, unitProperties;
+   var form, properties;

    validateParameterPresence( unit, "unit" );
    validateParameterTypePlainObject( options, "options" );

    form = options.form || "long";
-   compoundUnitPattern = unitGet( "per", form, this.cldr ).compoundUnitPattern;
-   unitProperties = unitGet( unit, form, this.cldr );
+   properties = {
+       compoundUnitPattern: this.cldr.main(["units", form, "per/compoundUnitPattern"])
+       unit: unitGet( unit, form, this.cldr )
+   }

-   return unitFormatterFn( unitProperties, this.pluralGenerator(), compoundUnitPattern );
+   return unitFormatterFn( properties, this.pluralGenerator() );

This comment has been minimized.

@rxaviers

rxaviers Oct 8, 2015

Member

Perhaps moving the code below in its own function, like unit/properties.js might avoid duplicate code in tests:

properties = {
    compoundUnitPattern: this.cldr.main(["units", form, "per/compoundUnitPattern"])
    unit: unitGet( unit, form, this.cldr )
}
@rxaviers

rxaviers Oct 8, 2015

Member

Perhaps moving the code below in its own function, like unit/properties.js might avoid duplicate code in tests:

properties = {
    compoundUnitPattern: this.cldr.main(["units", form, "per/compoundUnitPattern"])
    unit: unitGet( unit, form, this.cldr )
}
+var get = function( unit, form, cldr ) {
+ var ret;
+
+ // Ensure that we get the 'precomputed' form, if present.

This comment has been minimized.

@rxaviers

rxaviers Oct 8, 2015

Member

Let's include we're treating / like -per, something like:

Treat `/` like `-per-` and ensure that we get the 'precomputed' form, if present.
@rxaviers

rxaviers Oct 8, 2015

Member

Let's include we're treating / like -per, something like:

Treat `/` like `-per-` and ensure that we get the 'precomputed' form, if present.
src/unit/get.js
+ // Ensure that we get the 'precomputed' form, if present.
+ unit = unit.replace( /\//, "-per-" );
+
+ // Get unit or <type>-unit (eg. "duration-second").

This comment has been minimized.

@rxaviers

rxaviers Oct 8, 2015

Member

s/type/category for clarity (I know you inherited this issue 😄)

-   // Get unit or <type>-unit (eg. "duration-second").
+   // Get unit or <category>-unit (eg. "duration-second").
@rxaviers

rxaviers Oct 8, 2015

Member

s/type/category for clarity (I know you inherited this issue 😄)

-   // Get unit or <type>-unit (eg. "duration-second").
+   // Get unit or <category>-unit (eg. "duration-second").
@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Oct 8, 2015

Member

I've removed the below from the description since it's outdated. Please, correct me if I'm wrong.

The tests don't fully pass - line 56 in src/unit/format.js is causing problems:

    return globalize.formatPlural( value, ret );

I can dig in further, but if @rxaviers or someone else on the Globalize knows what API should be called now, that would be helpful.

Member

rxaviers commented Oct 8, 2015

I've removed the below from the description since it's outdated. Please, correct me if I'm wrong.

The tests don't fully pass - line 56 in src/unit/format.js is causing problems:

    return globalize.formatPlural( value, ret );

I can dig in further, but if @rxaviers or someone else on the Globalize knows what API should be called now, that would be helpful.

@alunny

This comment has been minimized.

Show comment
Hide comment
@alunny

alunny Oct 8, 2015

Collaborator

Yeah you're right to edit the description, the tests all pass now.

Collaborator

alunny commented Oct 8, 2015

Yeah you're right to edit the description, the tests all pass now.

@alunny alunny changed the title from WIP: rebase and merge unit formatting branch to Add unit formatting module Oct 8, 2015

@alunny

This comment has been minimized.

Show comment
Hide comment
@alunny

alunny Oct 8, 2015

Collaborator

I've added the module builds, runtime support, functional tests, and docs now. Phew!

Collaborator

alunny commented Oct 8, 2015

I've added the module builds, runtime support, functional tests, and docs now. Phew!

@@ -378,6 +378,22 @@ module.exports = function( grunt ) {
}
},
+ {
+ name: "globalize.unit",

This comment has been minimized.

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

GitHub has some kind of issue with this file. Looking at the source locally, I also get messed up syntax highlighting in my editor (SublimeText). Looks like an issue with the regex on line 192, completely unrelated to this PR though. Somehow this helps:

diff --git a/Gruntfile.js b/Gruntfile.js
index ce458fc..60819b1 100644
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -189,7 +189,7 @@ module.exports = function( grunt ) {
                             // Remove browserify wrappers.
                             .replace( /^\(function\(f\){if\(typeof exports==="object"&&type.*/, "" )
                             .replace( "},{}],2:[function(require,module,exports){", "" )
-                            .replace( /},{"\.\/messageformat-parser":1,"make-plural\/plural.*/, "" )
+                            .replace( /},{"\.\/messageformat-parser\":1,"make-plural\/plural.*/, "" )
                             .replace( /},{}\]},{},\[2\]\)\(2\)[\s\S]*?$/, "" )

                             // Set `MessageFormat.plurals` and remove `make-plural/plurals`
@jzaefferer

jzaefferer Oct 9, 2015

Contributor

GitHub has some kind of issue with this file. Looking at the source locally, I also get messed up syntax highlighting in my editor (SublimeText). Looks like an issue with the regex on line 192, completely unrelated to this PR though. Somehow this helps:

diff --git a/Gruntfile.js b/Gruntfile.js
index ce458fc..60819b1 100644
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -189,7 +189,7 @@ module.exports = function( grunt ) {
                             // Remove browserify wrappers.
                             .replace( /^\(function\(f\){if\(typeof exports==="object"&&type.*/, "" )
                             .replace( "},{}],2:[function(require,module,exports){", "" )
-                            .replace( /},{"\.\/messageformat-parser":1,"make-plural\/plural.*/, "" )
+                            .replace( /},{"\.\/messageformat-parser\":1,"make-plural\/plural.*/, "" )
                             .replace( /},{}\]},{},\[2\]\)\(2\)[\s\S]*?$/, "" )

                             // Set `MessageFormat.plurals` and remove `make-plural/plurals`

This comment has been minimized.

@rxaviers

rxaviers Oct 9, 2015

Member

@jzaefferer can you file a separate ticket for this please?

@rxaviers

rxaviers Oct 9, 2015

Member

@jzaefferer can you file a separate ticket for this please?

This comment has been minimized.

@rxaviers

rxaviers Oct 10, 2015

Member

for the record, #527 (fixed by #529)

@rxaviers

rxaviers Oct 10, 2015

Member

for the record, #527 (fixed by #529)

README.md
@@ -571,6 +574,29 @@ handle dependencies and CLDR loading manually yourself.
Alias for `.relativeTimeFormatter( unit, options )( value )`.
+## Unit module
+
+-- **`.unitFormatter( unit, options )`**

This comment has been minimized.

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

The two dashes at the start of the line should be a single dash, to match the formatting from other API entries.

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

The two dashes at the start of the line should be a single dash, to match the formatting from other API entries.

doc/api/unit/unit-formatter.md
+// > "3 months"
+```
+
+You can use the instance method `.relativeTimeFormatter()`, which uses the instance locale.

This comment has been minimized.

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

Copy&paste mistake? Seems like this should refer to formatUnit.

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

Copy&paste mistake? Seems like this should refer to formatUnit.

+
+**options**
+
+- form: [String] eg. "long", "short" or "narrow".

This comment has been minimized.

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

There are no options for formatting the number itself?

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

There are no options for formatting the number itself?

This comment has been minimized.

@rxaviers

rxaviers Oct 9, 2015

Member

Shoot. Good point.

I would add a numberFormatter option that takes a formatter as option.

@rxaviers

rxaviers Oct 9, 2015

Member

Shoot. Good point.

I would add a numberFormatter option that takes a formatter as option.

+ *
+ * Return all unit categories.
+ */
+return [ "acceleration", "angle", "area", "digital", "duration", "length", "mass", "power",

This comment has been minimized.

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

This is used in exactly one place, does that really justify a separate file?

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

This is used in exactly one place, does that really justify a separate file?

This comment has been minimized.

@rxaviers

rxaviers Oct 9, 2015

Member

Yes, it helps to make the build easier. This could change though when the source code uses ES6.

@rxaviers

rxaviers Oct 9, 2015

Member

Yes, it helps to make the build easier. This could change though when the source code uses ES6.

src/unit/format.js
+ *
+ * @pluralGenerator [Object]: A pluralGenerator from Globalize.pluralGenerator.
+ *
+ * TODO pass along numberFormatter

This comment has been minimized.

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

Is this tracked in a separate issue?

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

Is this tracked in a separate issue?

This comment has been minimized.

@rxaviers

rxaviers Oct 9, 2015

Member

Oh! So, here it is. I am ok to track this as a separate issue. I will let @alunny to answer if he is comfortable including this in this PR or to leave it to a subsequent one.

@rxaviers

rxaviers Oct 9, 2015

Member

Oh! So, here it is. I am ok to track this as a separate issue. I will let @alunny to answer if he is comfortable including this in this PR or to leave it to a subsequent one.

This comment has been minimized.

@rxaviers

rxaviers Oct 9, 2015

Member

Let me correct myself... The number needs to be formatted using numberFormatter() (instead of being passed directly to the formatMessage as is below) in this PR, but we can defer allowing users to pass a different numberFormatter() as an option to a subsequent PR.

Use relativeTimeFormatter as reference. But, note that I suggested a minor different thing than what relativeTimeFormatter does. It passes along its own options to the numberFormatter, which raises a new point: both should behave analogously.

I believe allowing a user to pass the formatter as an option is better than mixing options of both unit formatter and number formatter. What do you think?

@rxaviers

rxaviers Oct 9, 2015

Member

Let me correct myself... The number needs to be formatted using numberFormatter() (instead of being passed directly to the formatMessage as is below) in this PR, but we can defer allowing users to pass a different numberFormatter() as an option to a subsequent PR.

Use relativeTimeFormatter as reference. But, note that I suggested a minor different thing than what relativeTimeFormatter does. It passes along its own options to the numberFormatter, which raises a new point: both should behave analogously.

I believe allowing a user to pass the formatter as an option is better than mixing options of both unit formatter and number formatter. What do you think?

This comment has been minimized.

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

Since the unit formatter needs to access the actual number, passing a custom number formatter sounds fine to me. Would be good to include in this PR.

@jzaefferer

jzaefferer Oct 9, 2015

Contributor

Since the unit formatter needs to access the actual number, passing a custom number formatter sounds fine to me. Would be good to include in this PR.

This comment has been minimized.

@alunny

alunny Oct 9, 2015

Collaborator

I don't think it's too much trouble to pass a custom number formatter, I'll take a look at that today.

@alunny

alunny Oct 9, 2015

Collaborator

I don't think it's too much trouble to pass a custom number formatter, I'll take a look at that today.

@rxaviers rxaviers referenced this pull request Oct 10, 2015

Open

Docs: Use Globalize with Ecma-402 #532

0 of 2 tasks complete

@rxaviers rxaviers referenced this pull request in johnnyreilly/globalize-so-what-cha-want Oct 20, 2015

Closed

Unit module #3

+ util.assertParameterPresence( assert, "options", function( ) {
+ Globalize.formatUnit( 1, "day" );
+ });
+});

This comment has been minimized.

@rxaviers

rxaviers Oct 20, 2015

Member

Actually options should be optional. 😝

@rxaviers

rxaviers Oct 20, 2015

Member

Actually options should be optional. 😝

This comment has been minimized.

@rxaviers

rxaviers Oct 20, 2015

Member

I'm working on it.

@rxaviers

rxaviers Oct 20, 2015

Member

I'm working on it.

rxaviers added a commit that referenced this pull request Oct 20, 2015

rxaviers added a commit that referenced this pull request Oct 20, 2015

@rxaviers rxaviers closed this in 15a9dd2 Oct 20, 2015

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Oct 20, 2015

Member

Awesome! Rebased and merged. Thx @alunny

Member

rxaviers commented Oct 20, 2015

Awesome! Rebased and merged. Thx @alunny

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Oct 20, 2015

Member

Published in 1.1.0-rc.4

Member

rxaviers commented Oct 20, 2015

Published in 1.1.0-rc.4

@alunny

This comment has been minimized.

Show comment
Hide comment
@alunny

alunny Oct 20, 2015

Collaborator

Great, thanks @rxaviers!

Collaborator

alunny commented Oct 20, 2015

Great, thanks @rxaviers!

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 20, 2015

Contributor

👏 👏 👏

Contributor

jzaefferer commented Oct 20, 2015

👏 👏 👏

rxaviers added a commit that referenced this pull request Oct 21, 2015

rxaviers added a commit that referenced this pull request Oct 21, 2015

rxaviers added a commit that referenced this pull request Oct 21, 2015

rxaviers added a commit that referenced this pull request Oct 21, 2015

rxaviers added a commit that referenced this pull request Oct 21, 2015

rxaviers added a commit that referenced this pull request Feb 4, 2016

rxaviers added a commit that referenced this pull request Feb 4, 2016

rxaviers added a commit that referenced this pull request Feb 4, 2016

rxaviers added a commit that referenced this pull request Feb 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment