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

Dynamically augment availableFormats #462

Closed
wants to merge 3 commits into from
Closed

Conversation

@vectart
Copy link

vectart commented Jul 15, 2015

fixes #271

@vectart vectart force-pushed the vectart:master branch from ecf2647 to 049db10 Jul 15, 2015
@jquerybot jquerybot added CLA: Valid and removed CLA: Error labels Jul 15, 2015
Dynamically augment availableFormats

Fixes #271
@vectart vectart force-pushed the vectart:master branch from 049db10 to c2ae893 Jul 15, 2015
@rxaviers
Copy link
Member

rxaviers commented Jul 15, 2015

Awesome! All I see for now are some style issues, which I'm going to comment inline.

@@ -7,3 +7,4 @@ before_install:
install:
- npm install
- bower install
sudo: false

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 15, 2015

Member

cc @jzaefferer @scottgonzalez to comment on that

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Jul 15, 2015

Contributor

As long as its in a separate commit. Should also enable caching, see, for example, qunitjs/qunit#835

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 16, 2015

Member

@vectart please could you include this? ☝️


pattern = cldr.main([ path, skeleton ]);

if (skeleton && pattern === undefined) {

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 15, 2015

Member

Spaces if ( skeleton && pattern === undefined ) {

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 15, 2015

Member

If assume it's safe to use !pattern in here.

@vectart vectart force-pushed the vectart:master branch from 7ac6547 to 6312ba4 Jul 15, 2015
@@ -38,6 +39,89 @@ return function( options, cldr ) {
);
}

function getBestMatchPattern( path, skeleton ) {
var availableFormats, ratedFormats, format;

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 15, 2015

Member

I assume you want to define pattern as a variable local to this function, so it's not mixed up with pattern defined by the parent function.

availableFormats = cldr.main([ path ]);
ratedFormats = [];

for (format in availableFormats) {

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 15, 2015

Member

Spaces for ( format in availableFormats ) {

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 15, 2015

Member

There are more occurrences like this below...

}

function augmentFormat( requestedSkeleton, bestMatchFormat ) {
var originalBestMatchFormat, i, t, j, k, l;

This comment has been minimized.

requestedSkeleton = requestedSkeleton.match(datePatternRe);
bestMatchFormat = bestMatchFormat.match(datePatternRe);

for (i = 0, l = bestMatchFormat.length; i < l; i++) {

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 15, 2015

Member

@jquery/globalize what's your thought about it? Is it worth for performance reasons or it's safe to compare to length directly on each iteration?

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 15, 2015

Member

@vectart yeap, it's safe to compare to length directly for ( i = 0; i < bestMatchFormat.length; i++ ) {, length won't iterate through the array to calculate itself each time. So, you can get rid of 2 variables.

@rxaviers
Copy link
Member

rxaviers commented Jul 15, 2015

@vectart I left a couple of comments, please just let me know on any questions and thanks so far.

@vectart vectart force-pushed the vectart:master branch from 6312ba4 to aa6898d Jul 15, 2015
@vectart
Copy link
Author

vectart commented Jul 15, 2015

@rxaviers Thanks for the useful comments, I've updated PR according to them

@rxaviers
Copy link
Member

rxaviers commented Jul 16, 2015

Please, could you rebase on master (git rebase master)? I've updated jscs and it will help us to find the coding style issues.

maxLength = Math.max( formatA.length, formatB.length );
minLength = Math.min( formatA.length, formatB.length );

for ( index = 0; index < minLength; index++ ) {

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 16, 2015

Member

It's ok using i instead of index here. :)

});

if ( ratedFormats.length ) {
pattern = augmentFormat( skeleton, ratedFormats[0].pattern );

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 16, 2015

Member

space ratedFormats[ 0 ]

@rxaviers
Copy link
Member

rxaviers commented Jul 16, 2015

Interestingly, the augmentation for Globalize("en").formatDate(new Date(), {skeleton: "hhmm"}) worked just fine: '12:34 AM'. Although, it didn't work for Globalize("en").formatDate(new Date(), {skeleton: "HHmm"}): '0033' ('00:33'' expected). Please, could you check and also add this as a new test case?

@rxaviers
Copy link
Member

rxaviers commented Jul 16, 2015

Another worth doing test is checking against ICU... One difference I found is: EHmss gives 'Wed, 20:38:05' opposed to Wed 20:38:05 on ICU (http://demo.icu-project.org/icu4jweb/flexTest.jsp?pat=EHmss&_=en_US). The first one seems to have chosen by E alone and ms alone, then augmented ms into mss, then glued them. The second one seems to have chosen EHms, then augmented it into EHmss.

timeSkeleton = getBestMatchPattern(
"dates/calendars/gregorian/dateTimeFormats/availableFormats",
timeSkeleton
);

This comment has been minimized.

Copy link
@rxaviers

rxaviers Jul 16, 2015

Member

You might need use getBestMatchPattern() for each date and time before the type assignment above. For example, MMMMh should give July, 8 PM (type medium) instead of July at 8 PM (type long). Because, "MMMM" will be converted into "LLLL".

@rxaviers
Copy link
Member

rxaviers commented Jul 22, 2015

@vectart just let me know when you have progress, so we can land this fix. Thanks for your great work so far.

Strate added a commit to megaplan/globalize that referenced this pull request Mar 22, 2016
Strate added a commit to megaplan/globalize that referenced this pull request Mar 22, 2016
@Strate
Copy link
Contributor

Strate commented Apr 1, 2016

How about to use builtin new Intl.DateTimeFormatter().resolved.pattern to get best-fit pattern?

@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 added a commit to rxaviers/globalize that referenced this pull request 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.