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: Olson-timezone-support (real 'z', 'v', 'V') #687

Closed
wants to merge 1 commit into from

Conversation

rajavelmani
Copy link
Contributor

@rajavelmani rajavelmani commented Feb 9, 2017

#340

  • Supports z, v, V format with timezone names

Added by @rxaviers:

TODO:

  • Normal Code
  • Unit tests
    • test/unit/date/format.js
    • test/unit/date/format-properties.js
  • Functional tests
    • Test the eventual options.timeZone
  • Runtime code
  • Documentation
    • Document the eventual options.timeZone
  • Examples
    • Add the eventual options.timeZone

Follow up items:

  • Support the historic metazones based on the given date.

@jsf-clabot
Copy link

jsf-clabot commented Feb 9, 2017

CLA assistant check
All committers have signed the CLA.

@rajavelmani rajavelmani force-pushed the master branch 2 times, most recently from 212e748 to 8e074bf Compare Feb 10, 2017
Copy link
Member

@rxaviers rxaviers left a comment

@rajavelmani, thanks for your PR. I left some comments above and updated the description with some TODOs. Thanks so far!!

src/date.js Outdated
@@ -51,7 +51,7 @@ function validateRequiredCldr( path, value ) {
*/
Globalize.dateFormatter =
Globalize.prototype.dateFormatter = function( options ) {
var args, cldr, numberFormatters, pad, pattern, properties, returnFn;
var args, cldr, numberFormatters, pad, pattern, properties, returnFn, tzId;
Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

CLDR and IANA refers to them as timezone names. Let's rename tzId into timezoneName?

Copy link
Contributor Author

@rajavelmani rajavelmani Feb 10, 2017

Choose a reason for hiding this comment

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

i can use timeZone as timezoneName is locale dependent

src/date.js Outdated
@@ -66,6 +66,7 @@ Globalize.prototype.dateFormatter = function( options ) {
pattern = dateExpandPattern( options, cldr );
properties = dateFormatProperties( pattern, cldr );
cldr.off( "get", validateRequiredCldr );
properties.tzId = tzId;//TODO validate
Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

This variable is being declared and then set here (i.e., always undefined). I believe this was here as a placeholder to accept a potential options.timeZone value. Therefore, I think this can be safely removed for now.

);

cldr = new Cldr( "en" );

QUnit.assert.dateFormat = function( date, pattern, cldr, expected ) {
QUnit.assert.dateFormat = function( date, pattern, cldr, expected, tzId ) {
Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

Nitpick comment, let's leave cldr, expected as the last arguments. We could place tzId (actually, timezoneName) after pattern.

Copy link
Contributor Author

@rajavelmani rajavelmani Feb 10, 2017

Choose a reason for hiding this comment

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

Just to confirm, I added the tzId to the end to avoid the changes to rest of the test cases. if your are ok to change all the test then, i can change all the tests.

Copy link
Contributor Author

@rajavelmani rajavelmani Feb 10, 2017

Choose a reason for hiding this comment

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

I have changed the order of the signature as you mentioned.


//fall through to 'O' format
date = new FakeDate( 0 );
assert.dateFormat(date, "z", cldr, "GMT", "Etc/GMT");
Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

Where that is unavailable, falls back to the short localized GMT format ("O").
http://www.unicode.org/reports/tr35/tr35-dates.html#dfst-zone

According to the above, I expect a test where it's used an unavailable metaZone, not "Etc/GMT" that actually exists and is equal to O format as a coincidence.

Copy link
Contributor Author

@rajavelmani rajavelmani Feb 10, 2017

Choose a reason for hiding this comment

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

i used "America/Argentina/Buenos_Aires" which is not supported in CLDR json yet

Copy link
Member

@rxaviers rxaviers Feb 13, 2017

Choose a reason for hiding this comment

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

Nice! Though, I'm wondering... Should we use an invalid stuff that will never be supported like "Foo/Bar"?

);
properties.regionFormatDaylight = cldr.main(
"dates/timeZoneNames/regionFormat-type-daylight"
);
Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

Missing break here, i.e., in the case where it's found.

format = ( standardTzName ) ? standardTzName :
( regionFormatStandard ) ? regionFormatStandard.replace( /\{0\}/, exemplarCity ) :
undefined;
}
Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

It seems to me that at this runtime point we could have done more processing on these properties... I mean, standardTzName (or exemplarCity) could have been incorporated into regionFormatStandard, and daylightTzName (or exemplarCity) could have been incorporated into regionFormatDaylight.

Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

Other points:

  • The parentheses on ( standardTzName ) ? is unnecessary;
  • Why the undefined case?

Copy link
Contributor Author

@rajavelmani rajavelmani Feb 10, 2017

Choose a reason for hiding this comment

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

It seems to me that at this runtime point we could have done more processing on these properties... I mean, standardTzName (or exemplarCity) could have been incorporated into regionFormatStandard, and daylightTzName (or exemplarCity) could have been incorporated into regionFormatDaylight.

I have optimized the properties please take a look and let me know your thoughts

@@ -105,7 +105,8 @@ QUnit.test( "should allow for runtime compilation", function( assert ) {
"pm-alt-variant": "pm"
},
"pattern": "h:mm:ss a",
"timeSeparator": ":"
"timeSeparator": ":",
"tzId": undefined
Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

If it's undefined, let's simply not include it.

assert.dateFormat( date, "OO", cldr, "GMT+11" );
assert.dateFormat( date, "OOO", cldr, "GMT+11" );
assert.dateFormat( date, "OOOO", cldr, "GMT+11:00" );
});
Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

There's already a "should format timezone (O)" section above. Instead of duplicating it, let's simply incorporate the missing pieces there please.

// z...zzz: "{shortRegion}", eg. "PST" or "PDT".
// zzzz: "{regionName} {Standard Time}" or "{regionName} {Daylight Time}",
// eg. "Pacific Standard Time" or "Pacific Daylight Time".
if ( properties.tzId ) {
Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

Checking for properties.tzId isn't valid, because there's a case where properties.tzId is truthy and metaZone is unavailable and therefore the properties below are invalid (by looking at your code at date/format-properties.js). I think you should check for something else, e.g., one of the set properties.

Copy link
Contributor Author

@rajavelmani rajavelmani Feb 10, 2017

Choose a reason for hiding this comment

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

ok let me change this to check properties.standardTzName and properties.daylightTzName

);
if ( ret ) {
break;
}
Copy link
Member

@rxaviers rxaviers Feb 10, 2017

Choose a reason for hiding this comment

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

Since the properties exist, this should always be truthy.

Copy link
Contributor Author

@rajavelmani rajavelmani Feb 10, 2017

Choose a reason for hiding this comment

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

Yes, this will not be required with check for properties.standardTzName and properties.daylightTzName

@rajavelmani rajavelmani force-pushed the master branch 2 times, most recently from a0f3c96 to be210ba Compare Feb 11, 2017
length < 4 ? "short" : "long",
"daylight"
]);
if ( !standardTzName && !daylightTzName ) {
Copy link
Member

@rxaviers rxaviers Feb 13, 2017

Choose a reason for hiding this comment

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

What should happen for !standardTzName || !daylightTzName? It won't fall either in here or in the next if. Should we use examplarCity if !standardTzName || !daylightTzName. Probably, this would only happen in a corrupted CLDR data where there is one content, but not the other.

Copy link
Contributor Author

@rajavelmani rajavelmani Feb 14, 2017

Choose a reason for hiding this comment

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

this will help on the scenario like https://github.com/globalizejs/globalize/pull/687/files#diff-df6075946ae91cbb093f63c7597cd746R460 where the timezone id is valid not supported in CLDR.

Copy link
Member

@rxaviers rxaviers Feb 14, 2017

Choose a reason for hiding this comment

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

Note I'm referring to using || instead of && in here, so we also catch the additional case where part of the data exists.

Copy link
Member

@rxaviers rxaviers left a comment

Some more changes please

]);
if ( !standardTzName && !daylightTzName ) {
var exemplarCity = cldr.main(
"dates/timeZoneNames/zone/",
Copy link
Member

@rxaviers rxaviers Feb 13, 2017

Choose a reason for hiding this comment

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

remove trailing /

standardTzName = cldr.main(
"dates/timeZoneNames/regionFormat-type-standard"
)
.replace( /\{0\}/, exemplarCity );
Copy link
Member

@rxaviers rxaviers Feb 13, 2017

Choose a reason for hiding this comment

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

Instead of replacing {0} yourself, you can use src/common/format-message (example).

Copy link
Member

@rxaviers rxaviers Feb 13, 2017

Choose a reason for hiding this comment

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

FWIW, it's an internal utility to help in cases like that. Note, it's not ICU message format :)

Copy link
Contributor Author

@rajavelmani rajavelmani Feb 14, 2017

Choose a reason for hiding this comment

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

i just followed from the format.js. but, i can use the format-message as you suggested. Do i change the format.js code too?

var properties = {
numberFormatters: {},
pattern: pattern,
timeSeparator: numberSymbol( "timeSeparator", cldr )
timeSeparator: numberSymbol( "timeSeparator", cldr ),
timeZone: timeZone
Copy link
Member

@rxaviers rxaviers Feb 13, 2017

Choose a reason for hiding this comment

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

Let's skip adding timeZone to the properties. We can add it later if required.

Copy link
Contributor Author

@rajavelmani rajavelmani Feb 14, 2017

Choose a reason for hiding this comment

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

If we don't add timeZone from option.timeZone to properties.timeZone we may not be able to test time zone implementation. Can you clarify why we need to skip timeZone from properties?

assert.dateFormat(date, "zz", "America/Los_Angeles", cldr, "PST");
assert.dateFormat(date, "zzz", "America/Los_Angeles", cldr, "PST");
assert.dateFormat(date, "zzzz", "America/Los_Angeles", cldr, "Pacific Standard Time");

Copy link
Member

@rxaviers rxaviers Feb 13, 2017

Choose a reason for hiding this comment

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

We need to test the "exemplarCity" case.

@rajavelmani rajavelmani force-pushed the master branch 4 times, most recently from 5b0201e to 028ae5f Compare Feb 15, 2017
@rajavelmani rajavelmani force-pushed the master branch 3 times, most recently from 9599fe1 to b46372b Compare Feb 22, 2017
@rajavelmani rajavelmani changed the title Date: Olson-timezone-support (real z) Date: Olson-timezone-support (real 'z', 'v', 'V') Feb 23, 2017
@rajavelmani rajavelmani force-pushed the master branch 3 times, most recently from 801152a to 3c8928c Compare Mar 1, 2017
Copy link
Member

@rxaviers rxaviers left a comment

It's looking great!! I have left some comments for your consideration please.

}

//fall through "{0} {Time}""
//for VVVV format
Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

nitpick: space after //, like // for ...

//fall through "{0} {Time}""
//for VVVV format
if ( timeZone ) {
var TzName;
Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

nitpick: variable names should start with lowercase, i.e., tzName. Preferably using full words, like timezoneName.

]),
exemplarCity = cldr.main([
"dates/timeZoneNames/zone", timeZone, "exemplarCity"
]);
Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

If timeZone is passed to format anything but VVVV this is unnecessary, so let's move this assignment below in the only place where it's used.

metaZone = cldr.supplemental([
"metaZones/metazoneInfo/timezone", timeZone, 0,
"usesMetazone/_mzone"
]),
Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

If timeZone is passed to format anything but z* or v* this is unnecessary, so (considering we move exemplarCity below away) let's update the if condition:

- if ( timeZone ) {
+ if ( timeZone && ( chr === "v" || chr === "z" ) ) {

Copy link
Contributor Author

@rajavelmani rajavelmani Mar 2, 2017

Choose a reason for hiding this comment

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

assert.equal( "Pacific Time", properties( pattern, cldr, timeZone ).genericTzName );
}
});
});
Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

Let's add a test to assert vv and vvv throw.

Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

In here, or where it's possible to test this...

}
});
});

Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

Let's add a test to assert V throws.

@@ -208,6 +283,63 @@ return function( pattern, cldr ) {

// Zone
case "z":
properties.standardTzName = standardTzName;
properties.daylightTzName = daylightTzName;
break;
Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

nitpick: let's add an empty line after break and before the next case.

),
[ unKnownExemplarCity ]
);
}
Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

Can we simplify the the above as:

 					if ( length === 2 ) {
 						TzName = timeZone;
-					}
-
-					if ( exemplarCity ) {
-						if ( length === 3 ) {
-							TzName = exemplarCity;
-						} else if ( length === 4 ) {
-							TzName = formatMessage(
-								cldr.main(
-									"dates/timeZoneNames/regionFormat"
-								),
-								[ exemplarCity ]
-							);
+					} else if ( length === 3 ) {
+						TzName = exemplarCity;
+					} else if ( length === 4 ) {
+						if ( !exemplarCity ) {
+							exemplarCity = cldr.main([
+								"dates/timeZoneNames/zone/Etc/Unknown/exemplarCity"
+							]);
 						}
-					} else if ( !exemplarCity && length === 4 ) {
-						var unKnownExemplarCity = cldr.main([
-							"dates/timeZoneNames/zone/Etc/Unknown/exemplarCity"
-						]);
 						TzName = formatMessage(
 							cldr.main(
 								"dates/timeZoneNames/regionFormat"
 							),
-							[ unKnownExemplarCity ]
+							[ exemplarCity ]
 						);
 					}
 					if ( TzName ) {

Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

var date = new FakeDate( 0 );
assert.dateFormatWithTimezone( date, "VV", "America/Los_Angeles", cldr, "America/Los_Angeles" );
assert.dateFormatWithTimezone( date, "VVV", "America/Los_Angeles", cldr, "Los Angeles" );
assert.dateFormatWithTimezone( date, "VVVV", "America/Los_Angeles", cldr, "Los Angeles Time" );
Copy link
Member

@rxaviers rxaviers Mar 2, 2017

Choose a reason for hiding this comment

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

Can we include VVV and VVVV tests that use unkown city? It could re-use the above America/Argentina/Buenos_Aires, but it's good to have it explicitly tested here (in addition to the above fallback-test). Thanks

@rajavelmani rajavelmani force-pushed the master branch 2 times, most recently from ef85e52 to 8fd175f Compare Mar 2, 2017
*
* Load IANA data.
*/
Globalize.loadIANA = function( json ) {
Copy link
Member

@rxaviers rxaviers Mar 6, 2017

Choose a reason for hiding this comment

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

Pros:

  • Letting users to load this data similarly to the way they do with CLDR is good. Therefore we don't embed such in the library.

Cons:

  • Differently from CLDR, IANA doesn't provide with an official JSON binding. The one used here is non-official, it is using the JSON bindings from moment-timezone, and could be subject to changes. Ways to mitigate risks here could be 1) to get moment-timezone data put into a separate iana-json library; 2) check if IANA is interested in making it official? Ideas?

@@ -62,9 +80,12 @@ Globalize.prototype.dateFormatter = function( options ) {

validateDefaultLocale( cldr );

timeZone = options.timeZone;
validateParameterTypeString( options.timeZone, "options.timeZone" );
Copy link
Member

@rxaviers rxaviers Mar 6, 2017

Choose a reason for hiding this comment

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

Now that we the additional option options.timeZone, we need to update the above options = options || { skeleton: "yMd" }. For example, dateFormatter({timeZone: "something"}) should also set the default skeleton: "yMd". This default doesn't need to be set if either one is used: skeleton, date, or datetime.

@@ -3,9 +3,10 @@ define([
"./pattern-re",
"../common/create-error/unsupported-feature",
"../number/symbol",
"../util/string/pad"
"../util/string/pad",
"../common/format-message"
Copy link
Member

@rxaviers rxaviers Mar 6, 2017

Choose a reason for hiding this comment

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

nitpick: sort these filepaths alphabetically please, therefore common/format-message should be included after the common/.. one above.

"zones": [
{
"name": "America/Los_Angeles",
"abbrs": [
Copy link
Member

@rxaviers rxaviers Mar 6, 2017

Choose a reason for hiding this comment

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

Should we remove this?

420,
480
],
"population": 15058000
Copy link
Member

@rxaviers rxaviers Mar 6, 2017

Choose a reason for hiding this comment

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

Should we remove this?

});

QUnit.test( "should format timezone (v)", function( assert ) {
try {
Copy link
Member

@rxaviers rxaviers Mar 6, 2017

Choose a reason for hiding this comment

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

It seems like this was used for debug and was left?

@rxaviers
Copy link
Member

rxaviers commented Mar 8, 2017

Closed in favor of #701

@rxaviers rxaviers closed this Mar 8, 2017
rxaviers added a commit that referenced this pull request May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants