Fixed issue 272 Handle DateTime in same skeleton. #426

manrajgrover · 2015-03-18T21:31:44Z

Please review the code for issue #272 . I have tested it by making changes in date.js but not in expand-pattern.js . Please let me know the changes.

rxaviers · 2015-03-18T21:53:56Z

src/date/expand-pattern.js

-			result = cldr.main([
-				"dates/calendars/gregorian/dateTimeFormats/availableFormats",
-				pattern.skeleton
-			]);


and if the availableFormats data does not include a dateFormatItem whose skeleton matches the same set of fields

First, check for availableFormats. Therefore, keep the above. If result === undefined, keep going...

@rxaviers sir,

Sir here it is throwing error E_MISSING_CLDR error from cldr.main itself and not returning undefined for skeletons containing both date and time part. Hence we first have to check if skeleton entered has dateSkeleton or timeSkeleton or both before getting result. I am attaching a screenshot that will explain better.

Hence we first have to check if skeleton entered has dateSkeleton or timeSkeleton or both before getting result. I am attaching a screenshot that will explain better.

Nope. There may be skeletons containing both date and time parts. So, we need to follow what's in the specification, which says "if the availableFormats data does not include a dateFormatItem whose skeleton matches the same set of fields" that we should look for date and time separately.

We need to find a way to technically do that. cldr.main itself doesn't throw E_MISSING_CLDR error. The error is thrown by our validation. We could skip that path from the validation. See https://github.com/jquery/globalize/blob/master/src/date.js#L63.

For example, otherwise you wouldn't get EHms.

@rxaviers sir,

There may be skeletons containing both date and time parts. So, we need to follow what's in the specification, which says "if the availableFormats data does not include a dateFormatItem whose skeleton matches the same set of fields" that we should look for date and time separately.

Yeah. Some cases will get missed through this method.

We could skip that path from the validation. See https://github.com/jquery/globalize/blob/master/src/date.js#L63.

By skipping the path you mean adding the path for availableFormats here?
https://github.com/jquery/globalize/blob/master/src/date.js#L29

By skipping the path you mean adding the path for availableFormats here?
https://github.com/jquery/globalize/blob/master/src/date.js#L29

Correct.

rxaviers · 2015-03-18T22:08:20Z

Below is what needs to be done: (from the spec)

If a client-requested set of fields includes both date and time fields, and if the availableFormats data does not include a dateFormatItem whose skeleton matches the same set of fields, then the request should be handled as follows:

Divide the request into a date fields part and a time fields part

Can you point me where in your code is this being performed?

For each part, find the matching dateFormatItem

Can you point me where in your code is this being performed?

Combine the patterns for the two dateFormatItems using the appropriate dateTimeFormat pattern, determined as follows from the requested date fields:

If the requested date fields include wide month (MMMM, LLLL) and weekday name of any length (e.g. E, EEEE, c, cccc), use

Otherwise, if the requested date fields include wide month, use

Otherwise, if the requested date fields include abbreviated month (MMM, LLL), use

Otherwise use

rxaviers · 2015-03-18T22:11:29Z

Beyond the above comments, your code needs to follow our coding styles http://contribute.jquery.org/style-guide/js/.

rxaviers · 2015-03-18T22:13:22Z

src/date/expand-pattern.js

+	}
+	return result;
+}
+


By placing this function outside the define scope, it's outside the module and therefore won't build correctly.

Given you have created this function, why aren't you using it on case "datetime" in pattern: below? You have duplicate block of code.

Please, see my comment above (1) and (2) items.

@rxaviers sir,

Sir, there is difference in path used in cldr.main of blocks of code mentioned. If wanted, we can add a if block for case "datetime" in pattern: and check for "datetime" in pattern: or "skeleton" in pattern: to handle both in one function. Should I add?

manrajgrover · 2015-03-18T22:38:49Z

@rxaviers sir,

What I have inferred from the issue is this:

Divide the request into a date fields part and a time fields part

Here there is no need to do this since the date and time part can be checked in both A and B ways as mentioned by you above and we won't be requiring individual parts as the 4 cases below don't need them. If they are present individually, they have already been handled before.

For each part, find the matching dateFormatItem

Again since we have to return "full" , "long", "medium", "short" , I see no need to find raw patterns.

Combine the patterns for the two dateFormatItems using the appropriate dateTimeFormat pattern, determined as follows from the requested date fields

Here we need to return "full" , "long", "medium", "short" according to cases.

This is what I meant in the rough code I mentioned here

Now I feel I had inferred it wrong. What I now feel needs to be done is this:

Divide the request into a date fields part and a time fields part

Here extract the date and time part by parsing and checking for 'M', 'h' etc.

For each part, find the matching dateFormatItem

Check if these exist individually in the availableFormats. What if it doesn't? Throw an error, right?

Combine the patterns for the two dateFormatItems using the appropriate dateTimeFormat pattern, determined as follows from the requested date fields

Case 1. If date component has "(MMMM, LLLL)" and "(E, EEEE, c, cccc)" , we use rawPattern of "full". By use we mean return the "full" or we need to arrange "MMMM" and everything in component as in rawPattern of "full" etc, right? Need a suggestion how to do this, if it indeed is to be done.
....(same for next cases)

Beyond the above comments, your code needs to follow our coding styles http://contribute.jquery.org/style-guide/js/.

Okay sir, going through it.

manrajgrover · 2015-03-19T22:09:01Z

@rxaviers sir,

May I know if new inference is correct or if there is any changes required.

manrajgrover · 2015-03-20T16:10:37Z

@rxaviers sir,

If there is any change in what I mentioned please let me know or give me a yes. so that I start implementing it. Thank you. Sorry if I am disturbing.

rxaviers · 2015-03-20T17:25:03Z

@manrajgrover, I'm not convinced your interpretation is correct. What pattern do you get for the skeleton "MMME"? Do you get "LLL, ccc"?

manrajgrover · 2015-03-21T01:47:26Z

@rxaviers sir,

Nope. But I guess I have started to see your point now and what the algorithm actually says.

If a client-requested set of fields includes both date and time fields, and if the availableFormats data does not include a dateFormatItem whose skeleton matches the same set of fields, then the request should be handled as follows:

Divide the request into a date fields part and a time fields part

We need to take out the date and time from the skeleton provided.

For each part, find the matching dateFormatItem

We match the dateSkeleton and timeSkeleton to the availableFormats

Combine the patterns for the two dateFormatItems using the appropriate dateTimeFormat pattern, determined as follows from the requested date fields:

If the requested date fields include wide month (MMMM, LLLL) and weekday name of any length (e.g. E, EEEE, c, cccc), use <dateTimeFormatLength type="full">

Sir here we will check dateSkeleton and see if it contains MMMM and if yes then we will use "full" dateTime skeleton, convert it to raw pattern and then map the raw pattern of dateSkeleton to dateTime raw pattern and delete which all unmapped entities from result before returning it.

   1. Otherwise, if the requested date fields include wide month, use `<dateTimeFormatLength type="long">`

Same here with dateTime raw pattern "long".

   1. Otherwise, if the requested date fields include abbreviated month (MMM, LLL), use `<dateTimeFormatLength type="medium">`

Same here with dateTime raw pattern "medium".

   1. Otherwise use `<dateTimeFormatLength type="short">`

Same here with dateTime raw pattern "short".

Sorry for taking time to understand this. I hope this is correct now.

rxaviers · 2015-03-21T15:40:27Z

We need to take out the date and time from the skeleton provided.

Correct. 👍

We match the dateSkeleton and timeSkeleton to the availableFormats

Correct. 👍

Sir here we will check dateSkeleton and see if it contains MMMM and if yes then we will use "full" dateTime skeleton, convert it to raw pattern and then map the raw pattern of dateSkeleton to dateTime raw pattern and delete which all unmapped entities from result before returning it.
...
Same here with dateTime raw pattern "long".
...
Same here with dateTime raw pattern "medium".
...
Same here with dateTime raw pattern "short".

I don't think this is correct. I understood that depending of the fields present in the dateSkeleton or timeSkeleton you should use a different form of "glue". For example, if the skeleton has "MMMM", you must use <dateTimeFormatLength type="full">, which is {1} 'at' {0}. You will replace {1} with the date pattern and {0} with the time pattern according to http://www.unicode.org/reports/tr35/tr35-dates.html#dateTimeFormat.

Sorry for taking time to understand this. I hope this is correct now.

No problem. Thanks for the resilience.

manrajgrover · 2015-03-22T06:20:47Z

@rxaviers sir,

Ohkay sir! I was thinking of giving full flexibility to user to use any skeleton he/she wants and then mapping accordingly to give the result(This would require some brainstorming as well)

I am currently working on it but one thing that is confusing is availableFormats does not contain any skeleton for MMMM. I will add this case once this is cleared. Till then, I will code rest of the things.

manrajgrover · 2015-03-22T09:47:57Z

@rxaviers sir,

I have re-coded the whole feature and tested on few cases and got positive results. I have also taken care of putting G to the end of the result in case of medium and short for putting AD at the end. I have tried to follow good coding style given here but don't know if I am missing anything or not.

Also please clear the MMMM case as they are not available under availableFormats given here.

I have not included L and c cases as they don't occur in skeleton but occur in raw pattern.

Please review the code. Thank you.

jzaefferer · 2015-03-23T19:46:48Z

@manrajgrover regarding GSoC (I missed your IRC PM), please submit your proposal on the official site, we can review it there.

manrajgrover · 2015-03-23T20:09:31Z

@jzaefferer sir,

No problem sir. Sure sir. Thank you. :)

manrajgrover · 2015-03-28T06:36:56Z

@rxaviers sir,

I feel everything is covered now. Please review the code and let me know if I am missing anything. Thank you.

rxaviers · 2015-03-29T16:25:44Z

src/date.js

@@ -29,7 +29,8 @@ function validateRequiredCldr( path, value ) {
 		skip: [
 			/dates\/calendars\/gregorian\/days\/.*\/short/,
 			/supplemental\/timeData\/(?!001)/,
-			/supplemental\/weekData\/(?!001)/
+			/supplemental\/weekData\/(?!001)/,
+			/dates\/calendars\/gregorian\/dateTimeFormats\/availableFormats/


Style: please sort it using alphabetical order, i.e., as first line.

manrajgrover · 2015-03-30T23:10:12Z

@rxaviers sir,

Made the changes. Please review the fixes. Thank you.

manrajgrover · 2015-04-04T04:11:19Z

Hi @rxaviers sir,

Please review the code and let me know the changes. Sorry if you're busy and I am disturbing. Thank you.

rxaviers · 2015-04-04T15:08:59Z

src/date.js

@@ -28,6 +28,7 @@ function validateRequiredCldr( path, value ) {
 	validateCldr( path, value, {
 		skip: [
 			/dates\/calendars\/gregorian\/days\/.*\/short/,
+			/dates\/calendars\/gregorian\/dateTimeFormats\/availableFormats/,
 			/supplemental\/timeData\/(?!001)/,
 			/supplemental\/weekData\/(?!001)/


Alphabetical order is:

/dates\/calendars\/gregorian\/dateTimeFormats\/availableFormats/, /dates\/calendars\/gregorian\/days\/.*\/short/, /supplemental\/timeData\/(?!001)/, /supplemental\/weekData\/(?!001)/

Oopsie. Fixing.

rxaviers · 2015-04-13T13:27:01Z

src/date/expand-pattern.js

 return function( pattern, cldr ) {
-	var result;
+	var result, getDateTime, skeleton, dateSkeleton = "", timeSkeleton = "", type;


- var result, getDateTime, skeleton, dateSkeleton = "", timeSkeleton = "", + var getDateTime, result, skeleton, type, + dateSkeleton = "", + timeSkeleton = "";

Note that variables that assign initial value are placed in their own lines. Note the alphabetical order.

rxaviers · 2015-04-13T14:11:15Z

Implementation is working, great! Now, it's time to fix the coding style issues for which I've left comments inline and consider replacing getDateTime for combineDateTime detailed in my inline comments as well.

manrajgrover · 2015-04-13T21:02:28Z

@rxaviers sir,

I have replaced getDateTime for combineDateTime but found a bug in the function and have commented issue and fix for it. Let me know if I missed anything.

rxaviers · 2015-04-14T11:55:19Z

@manrajgrover, yeap date and time were inverted there. Your fix is correct.

Ref #272 Ref #426

rxaviers · 2015-04-14T12:14:38Z

Landed thanks

manrajgrover · 2015-04-14T12:16:27Z

@rxaviers sir,

Great! Thank you for helping me throughout the fix. Really learn't a lot including coding styles through this fix. I feel we should document the new feature now and add examples for the same.

rxaviers · 2015-04-14T12:28:00Z

Sounds good. Feel free to send a PR with such updates. Thanks

Fixed issue 272 Handle DateTime in same skeleton.

f624add

jquerybot added the CLA: Valid label Mar 18, 2015

rxaviers reviewed Mar 18, 2015
View reviewed changes

manrajgrover added 4 commits March 22, 2015 14:51

Fixed code with new approach.

ddd187a

Minor fixes.

ca8489a

Fixed few style errors.

0e459b7

Some more style fixes.

93cb191

manrajgrover added 5 commits March 22, 2015 15:21

Some more fixes.

3b50fcd

Even more fixes.

f325299

Style fixes.

306b57e

Build style fixes.

45a2ed8

Some more build fixes.

2919ab6

rxaviers reviewed Mar 29, 2015
View reviewed changes

manrajgrover added 3 commits March 31, 2015 04:00

Fixes issues and alternatives.

e573063

Fixes minor error.

4876f8d

Fixes scope of function.

d64b3dc

Fixes whitespaces.

e9984fa

rxaviers reviewed Apr 4, 2015
View reviewed changes

manrajgrover added 2 commits April 4, 2015 21:26

Changed Alphabetical Order.

5c096d5

Removes duplicate block of code

6d0f96c

rxaviers reviewed Apr 13, 2015
View reviewed changes

manrajgrover added 2 commits April 14, 2015 02:19

Fixes code styling and issues related to getDateTime function.

6fce167

Fixes styling.

8929507

rxaviers closed this in c471268 Apr 14, 2015

rxaviers added a commit that referenced this pull request Apr 14, 2015

Test: Include unit test for expanding datetime skeletons

68d6643

Ref #272 Ref #426

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed issue 272 Handle DateTime in same skeleton. #426

Fixed issue 272 Handle DateTime in same skeleton. #426

manrajgrover commented Mar 18, 2015

rxaviers Mar 18, 2015

manrajgrover Mar 26, 2015

rxaviers Mar 26, 2015

rxaviers Mar 26, 2015

manrajgrover Mar 27, 2015

rxaviers Mar 27, 2015

rxaviers commented Mar 18, 2015

rxaviers commented Mar 18, 2015

rxaviers Mar 18, 2015

rxaviers Mar 29, 2015

manrajgrover Mar 30, 2015

manrajgrover commented Mar 18, 2015

manrajgrover commented Mar 19, 2015

manrajgrover commented Mar 20, 2015

rxaviers commented Mar 20, 2015

manrajgrover commented Mar 21, 2015

rxaviers commented Mar 21, 2015

manrajgrover commented Mar 22, 2015

manrajgrover commented Mar 22, 2015

jzaefferer commented Mar 23, 2015

manrajgrover commented Mar 23, 2015

manrajgrover commented Mar 28, 2015

rxaviers Mar 29, 2015

manrajgrover commented Mar 30, 2015

manrajgrover commented Apr 4, 2015

rxaviers Apr 4, 2015

manrajgrover Apr 4, 2015

rxaviers Apr 13, 2015

rxaviers commented Apr 13, 2015

manrajgrover commented Apr 13, 2015

rxaviers commented Apr 14, 2015

rxaviers commented Apr 14, 2015

manrajgrover commented Apr 14, 2015

rxaviers commented Apr 14, 2015

Fixed issue 272 Handle DateTime in same skeleton. #426

Fixed issue 272 Handle DateTime in same skeleton. #426

Conversation

manrajgrover commented Mar 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxaviers commented Mar 18, 2015

rxaviers commented Mar 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manrajgrover commented Mar 18, 2015

manrajgrover commented Mar 19, 2015

manrajgrover commented Mar 20, 2015

rxaviers commented Mar 20, 2015

manrajgrover commented Mar 21, 2015

rxaviers commented Mar 21, 2015

manrajgrover commented Mar 22, 2015

manrajgrover commented Mar 22, 2015

jzaefferer commented Mar 23, 2015

manrajgrover commented Mar 23, 2015

manrajgrover commented Mar 28, 2015

Choose a reason for hiding this comment

manrajgrover commented Mar 30, 2015

manrajgrover commented Apr 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxaviers commented Apr 13, 2015

manrajgrover commented Apr 13, 2015

rxaviers commented Apr 14, 2015

rxaviers commented Apr 14, 2015

manrajgrover commented Apr 14, 2015

rxaviers commented Apr 14, 2015