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

DateTimeFormatter: Dynamically augment skeleton #604

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Strate
Contributor

Strate commented Apr 3, 2016

Fixes #271
This is rework of #462.

I have implemented another skeleton pattern comparison algorithm, which fixes a lot of issues I have found in #462 implementation.

Also I have added a lot of tests, which were tested manually across Intl.DateTimeFormat and ICU Flex Test.

But not all resulst same from Flex Test and my implementation. This is difference table:

Skeleton Flex Pattern Globalize Pattern Comment
GyMMMEd EEE, MMM d, y G E, MMM d, y G There is explicit available format in cldr-data, which used by globalize
GyMMMEdhms EEE, MMM d, y G, h:mm:ss a E, MMM d, y G, h:mm:ss a This is combined GyMMMEd and hms patterns, which are both available in cldr-data and merged with dateTimeFormat.medium
hhmm h:mm a hh:mm a Globalize expand matched h:mm a to hh:mm a because user requested hh in his pattern
EHmms EEE HH:mm:ss E HH:mm:ss There is explicit entry in cldr-data, which matches EHms to E HH:mm:ss. Because in result pattern mm already present, we don't need to augment any more.

And all other differences are similar. In fact, I am really can not understand, why flexTest augments E to EEE, if you know why please tell me :-)

vectart and others added some commits Jul 15, 2015

Component: Date formatter
Dynamically augment availableFormats

Fixes #271
DateTimeFormatter: augment skeleton refactoring
Rework of `getBestMatchPattern` algorithm
 - properly handle close parts of sekletons, i.e. `L` and `M`
 - better distance calculating
Call to `getBestMatchPattern` before splitting skeleton to date and time parts
Add a lot of test cases to `expandPattern` function.

Fixes #271
@jquerybot

This comment has been minimized.

Show comment
Hide comment
@jquerybot

jquerybot Apr 3, 2016

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

jquerybot commented Apr 3, 2016

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

@jquerybot jquerybot added the CLA: Error label Apr 3, 2016

@Strate Strate changed the title from Dynamically augment skeleton to DateTimeFormatter: Dynamically augment skeleton Apr 3, 2016

@jquerybot jquerybot added CLA: Valid and removed CLA: Error labels Apr 3, 2016

Strate added a commit to megaplan/globalize that referenced this pull request Apr 3, 2016

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Apr 3, 2016

Member

@Strate, fantastic thanks for your contribution. Please, have you used any external reference (e.g., UTS#35) for implementing your algorithm? If so, could you add internal comments with some references please?

Member

rxaviers commented Apr 3, 2016

@Strate, fantastic thanks for your contribution. Please, have you used any external reference (e.g., UTS#35) for implementing your algorithm? If so, could you add internal comments with some references please?

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Apr 3, 2016

Contributor

@rxaviers My algorithm is close to http://www.unicode.org/reports/tr35/tr35-dates.html#Matching_Skeletons, but seems with several bugs in it. I do not handle rule "Numeric and text fields are given a larger distance from each other. MMM ≈ MM"

I'll work on it but later (next week I suppose)

Contributor

Strate commented Apr 3, 2016

@rxaviers My algorithm is close to http://www.unicode.org/reports/tr35/tr35-dates.html#Matching_Skeletons, but seems with several bugs in it. I do not handle rule "Numeric and text fields are given a larger distance from each other. MMM ≈ MM"

I'll work on it but later (next week I suppose)

result = result + str;
}
return result;
}

This comment has been minimized.

@rxaviers

rxaviers Apr 4, 2016

Member

Let's move this function into ./src/util/string/repeat-str.js. Maybe ./src/util/string/pad.js could reuse it.

@rxaviers

rxaviers Apr 4, 2016

Member

Let's move this function into ./src/util/string/repeat-str.js. Maybe ./src/util/string/pad.js could reuse it.

This comment has been minimized.

@Strate

Strate Apr 4, 2016

Contributor

Maybe we could use String.prototype.repeat with some of es6-shim?

@Strate

Strate Apr 4, 2016

Contributor

Maybe we could use String.prototype.repeat with some of es6-shim?

@@ -48,25 +171,38 @@ return function( options, cldr ) {
if ( !result ) {
timeSkeleton = skeleton.split( /[^hHKkmsSAzZOvVXx]/ ).slice( -1 )[ 0 ];
dateSkeleton = skeleton.split( /[^GyYuUrQqMLlwWdDFgEec]/ )[ 0 ];

This comment has been minimized.

@rxaviers

rxaviers Apr 4, 2016

Member

Let's defer this initialization by moving them both inside the else below that will actually use them.

@rxaviers

rxaviers Apr 4, 2016

Member

Let's defer this initialization by moving them both inside the else below that will actually use them.

} else {
result = dateSkeleton || timeSkeleton;
}
}

This comment has been minimized.

@rxaviers

rxaviers Apr 4, 2016

Member

In general, the whole block above is getting too indented. Let's change it. Currently, we have something like this:

tryDirectMap()
if (!directMap) {
  tryToMatchIt()
  if (!match) {
    tryToMatchItsIndividualParts()
  }
}

Let's re-arrange it so we have:

tryDirectMap()
if (directMap) {
  break;
}

tryToMatchIt()
if (match) {
  break;
}

tryToMatchItsIndividualParts()
@rxaviers

rxaviers Apr 4, 2016

Member

In general, the whole block above is getting too indented. Let's change it. Currently, we have something like this:

tryDirectMap()
if (!directMap) {
  tryToMatchIt()
  if (!match) {
    tryToMatchItsIndividualParts()
  }
}

Let's re-arrange it so we have:

tryDirectMap()
if (directMap) {
  break;
}

tryToMatchIt()
if (match) {
  break;
}

tryToMatchItsIndividualParts()

This comment has been minimized.

@rxaviers

rxaviers Apr 4, 2016

Member

About the directMap part of it... It's no longer needed, your getBestMatchPattern already checks for that.

@rxaviers

rxaviers Apr 4, 2016

Member

About the directMap part of it... It's no longer needed, your getBestMatchPattern already checks for that.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers
Member

rxaviers commented Apr 4, 2016

}
return bestMatchFormat.join( "" );
}

This comment has been minimized.

@rxaviers

rxaviers Apr 4, 2016

Member

In general about the functions above, can .match(datePatternRe) and normalizePatternType be made in advance in the caller function so that you don't need to execute these over and over?

@rxaviers

rxaviers Apr 4, 2016

Member

In general about the functions above, can .match(datePatternRe) and normalizePatternType be made in advance in the caller function so that you don't need to execute these over and over?

This comment has been minimized.

@Strate

Strate Apr 4, 2016

Contributor

You mean some sort of caching? But this could be a memory issue...

@Strate

Strate Apr 4, 2016

Contributor

You mean some sort of caching? But this could be a memory issue...

@jawsthegame

This comment has been minimized.

Show comment
Hide comment
@jawsthegame

jawsthegame Jun 20, 2016

Any movement on this? I could really use this fix.

jawsthegame commented Jun 20, 2016

Any movement on this? I could really use this fix.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Jun 20, 2016

Member

It is stalled. Would you like to take it over? I'd be happy if you could address the above comments. You could create a new pull request on top of this one with additional commits. Just let me know if you have any questions. Thanks

Member

rxaviers commented Jun 20, 2016

It is stalled. Would you like to take it over? I'd be happy if you could address the above comments. You could create a new pull request on top of this one with additional commits. Just let me know if you have any questions. Thanks

@JSFOwner JSFOwner removed the CLA: Valid label Nov 2, 2016

rxaviers added a commit to rxaviers/globalize that referenced this pull request Mar 14, 2017

rxaviers added a commit to rxaviers/globalize that referenced this pull request Mar 14, 2017

rxaviers added a commit to rxaviers/globalize that referenced this pull request Mar 14, 2017

@rxaviers rxaviers closed this in 687207b Mar 17, 2017

@rxaviers rxaviers added this to the 1.3.0 milestone Mar 17, 2017

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