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

Date: Add fractional seconds support in formatters #763

Merged
merged 6 commits into from Jul 17, 2018

Conversation

Projects
None yet
3 participants
@mattyork
Copy link
Contributor

mattyork commented Aug 3, 2017

@jsf-clabot

This comment has been minimized.

Copy link

jsf-clabot commented Aug 3, 2017

CLA assistant check
All committers have signed the CLA.

@@ -29,7 +29,7 @@ module.exports = {

{ formatter: Globalize.dateFormatter({ skeleton: "GyMMMEd" }), args: [ date ] },
{ formatter: Globalize.dateFormatter({ skeleton: "dhms" }), args: [ date ] },
{ formatter: Globalize.dateFormatter({ skeleton: "GyMMMEdhms" }), args: [ date ] },
{ formatter: Globalize.dateFormatter({ skeleton: "GyMMMEdhmsSSS" }), args: [ date ] },

This comment has been minimized.

@rxaviers

rxaviers Aug 3, 2017

Member

Let's keep the original test without S and have the additional test with S.

This comment has been minimized.

@mattyork

mattyork Aug 3, 2017

Author Contributor

sounds good!

}

return bestMatchFormat.join( "" );
return bestMatchFormatParts.join( "" );

This comment has been minimized.

@rxaviers

rxaviers Aug 3, 2017

Member

Good name addition, i.e., Parts which they are.

for ( j = 0; j < requestedSkeleton.length; j++ ) {
requestedType = requestedSkeleton[j].charAt( 0 );
requestedLength = requestedSkeleton[j].length;
requestedSkeletonParts = skeletonWithoutFractionalSeconds.match( datePatternRe );

This comment has been minimized.

@rxaviers

rxaviers Aug 3, 2017

Member

I'm wondering how this approach scales... Maybe, not worth worrying about it now, but for example, I'm wondering if handling timeZone would take a similar approach.

This comment has been minimized.

@rxaviers

rxaviers Aug 3, 2017

Member

Does it make sense to leave this function mostly as is, and handle S by a parent function? Basically, by replacing the existing call stack:

- getBestMatchPattern() -> augmentFormat()
+ getBestMatchPattern() -> stuffThatHandlesS() -> augmentFormat()

This comment has been minimized.

@mattyork

mattyork Aug 3, 2017

Author Contributor

Yeah, that makes sense. Is it cool if I keep both of those functions in augment-format.js? That way the file perfectly reflects the "adjustment" section of the spec:

Once a skeleton match is found, the corresponding pattern is used, but with adjustments. Consider the following dateFormatItem:

<dateFormatItem id="yMMMd">d MMM y</dateFormatItem>

If this is the best match for yMMMMd, pattern is automatically expanded to produce the pattern "d MMMM y" in response to the request. Of course, if the desired behavior is that a request for yMMMMd should produce something other than "d MMMM y", a separate dateFormatItem must be present, for example:

<dateFormatItem id="yMMMMd">d 'de' MMMM 'de' y</dateFormatItem>

If the requested skeleton included both seconds and fractional seconds and the dateFormatItem skeleton included seconds but not fractional seconds, then the seconds field of the corresponding pattern should be adjusted by appending the locale’s decimal separator, followed by the sequence of ‘S’ characters from the requested skeleton.

@@ -30,7 +30,7 @@ QUnit.test( "should get best match pattern", function( assert ) {
assert.equal( getBestMatchPattern( en, "cccd" ), "d EEE" );
assert.equal( getBestMatchPattern( en, "ccccd" ), "d EEEE" );

assert.equal( getBestMatchPattern( en, "hhmms" ), "hh:mm:ss a" );
assert.equal( getBestMatchPattern( en, "hhmmsS" ), "hh:mm:ss.S a" );

This comment has been minimized.

@rxaviers

rxaviers Aug 3, 2017

Member

Can we preserve the original test and have yours as an additional one?

This comment has been minimized.

@mattyork

mattyork Aug 3, 2017

Author Contributor

Yep yep!

}

// See: http://www.unicode.org/reports/tr35/tr35-dates.html#Matching_Skeletons
return function( requestedSkeleton, bestMatchFormat, decimalSeparator ) {

This comment has been minimized.

@mattyork

mattyork Aug 4, 2017

Author Contributor

@rxaviers how about this? Same file, two functions. Encapsulates all the "adjustments" to the bestMatchFormat

This comment has been minimized.

@rxaviers

rxaviers Aug 4, 2017

Member

It's good except for our build process, which assumes all files have one export (and no outer code). In order to have something different, we need an update the onBuildWrite, specifically tell it this file is a "type b".

This comment has been minimized.

@mattyork

mattyork Aug 4, 2017

Author Contributor

Got it. There's a problem though: if I make it a "type b", that capitalizes the first character of the variable name, which breaks the output, as the actual variable name does not have a capital first letter. Example output:

var DateExpandPatternAugmentFormat = (function() {
function expandBestMatchFormat( skeletonWithoutFractionalSeconds, bestMatchFormat ) {
    ...
}

// See: http://www.unicode.org/reports/tr35/tr35-dates.html#Matching_Skeletons
return function( requestedSkeleton, bestMatchFormat, decimalSeparator ) {
    ...
    bestMatchFormat = expandBestMatchFormat( skeletonWithoutFractionalSeconds, bestMatchFormat );
    ...
};

}());

Why does it capitalize the first letter in DateExpandPatternAugmentFormat? The only other "type b" is "util/globalize-date", but that file doesn't seem to exist anymore, so I can't look at the example to know what the intention of the capitalization is.

However, if I leave it as a "type a", everything still works. This is the output:

function expandBestMatchFormat( skeletonWithoutFractionalSeconds, bestMatchFormat ) {
    ...
}

var dateExpandPatternAugmentFormat = function( requestedSkeleton, bestMatchFormat, decimalSeparator ) {
    ...
    bestMatchFormat = expandBestMatchFormat( skeletonWithoutFractionalSeconds, bestMatchFormat );
    ...
};

This works just fine.

So, what should I do here?

  1. Leave as a type a
  2. Change how the build handles type b so it stops capitalizing. If I do this should I also remove the reference to util/globalize-date?
  3. Something else? Move the other function to another file?

This comment has been minimized.

@mattyork

mattyork Aug 8, 2017

Author Contributor

This comment has been minimized.

@rxaviers

rxaviers Aug 14, 2017

Member

Number 2. (sorry for the delay)

Yeap, existing "type b" is bugged. Let's fix it by stop capitalizing the variable and by also removing the reference to the non-existing util/globalize-date. Please, take a look at this old-but-correct "type b" implementation: here and here.

Thanks!

This comment has been minimized.

@mattyork

mattyork Aug 14, 2017

Author Contributor

Done!

mattyork added some commits Aug 4, 2017

@mattyork

This comment has been minimized.

Copy link
Contributor Author

mattyork commented Aug 14, 2017

How bout meow?

@@ -88,7 +88,7 @@ QUnit.test( "should return a formatter", function( assert ) {

assert.equal( Globalize.dateFormatter({ skeleton: "GyMMMEd" })( date ), "Wed, Sep 15, 2010 AD" );
assert.equal( Globalize.dateFormatter({ skeleton: "dhms" })( date ), "15, 5:35:07 PM" );
assert.equal( Globalize.dateFormatter({ skeleton: "GyMMMEdhms" })( date ), "Wed, Sep 15, 2010 AD, 5:35:07 PM" );
assert.equal( Globalize.dateFormatter({ skeleton: "GyMMMEdhmsSSS" })( date ), "Wed, Sep 15, 2010 AD, 5:35:07.369 PM" );

This comment has been minimized.

@rxaviers

rxaviers Aug 15, 2017

Member

Let's not replace the above test, but add this as a new one.

date = startOf( date, "second" );
assertParseDate( assert, "Wed 5:35:07 PM", { skeleton: "Ehms" }, date );
date.setMilliseconds(734);
assertParseDate( assert, "Wed 5:35:07.734 PM", { skeleton: "EhmsSSS" }, date );

This comment has been minimized.

@rxaviers

rxaviers Aug 15, 2017

Member

Same here...

@rxaviers

This comment has been minimized.

Copy link
Member

rxaviers commented Aug 15, 2017

Great @mattyork, one more test update and I think this is it.

@mattyork

This comment has been minimized.

Copy link
Contributor Author

mattyork commented Aug 16, 2017

Aye aye, tests updated

@mattyork

This comment has been minimized.

Copy link
Contributor Author

mattyork commented Sep 7, 2017

Poke

@mattyork

This comment has been minimized.

Copy link
Contributor Author

mattyork commented Sep 16, 2017

@rxaviers

This comment has been minimized.

Copy link
Member

rxaviers commented Sep 22, 2017

Hi @mattyork, I didn't forget about you. Sorry about the delay. I will merge your PR. I hope the delay doesn't impact any of your work.

@rxaviers rxaviers added this to the 1.4.0 milestone Mar 9, 2018

@rxaviers rxaviers merged commit dceb8cd into globalizejs:master Jul 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.