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

[bugfix] Fixes #3805, fix locale month getters for standalone/format cases #3806

Closed
wants to merge 12 commits into from
Closed

Conversation

timfish
Copy link
Contributor

@timfish timfish commented Mar 4, 2017

@jsf-clabot
Copy link

jsf-clabot commented Mar 4, 2017

CLA assistant check
All committers have signed the CLA.

@icambron
Copy link
Member

icambron commented Mar 5, 2017

This makes sense to me. Sounds like we should add a test that checks that each locale returns an array for monthsShort

@timfish
Copy link
Contributor Author

timfish commented Mar 5, 2017

I'm adding the following test to all 113 locales:

test('valid localeData', function (assert) {
    assert.equal(moment().localeData().months().length, 12, 'months should return 12 months');
    assert.equal(moment().localeData().monthsShort().length, 12, 'monthsShort should return 12 months');
    assert.equal(moment().localeData().weekdays().length, 7, 'weekdays should return 7 days');
    assert.equal(moment().localeData().weekdaysShort().length, 7, 'weekdaysShort should return 7 days');
    assert.equal(moment().localeData().weekdaysMin().length, 7, 'monthsShort should return 7 days');
});

Even after my fixes to es es-do nl nl-be fy there are 15 failing tests.
Have I opened a can of worms?

Module: locale:be Test: valid localeData
months should return 12 months
Actual value:undefined
Expected value:12

Module: locale:be Test: valid localeData
weekdays should return 7 days
Actual value:undefined
Expected value:7

Module: locale:el Test: valid localeData
Died on test #1     at global.test.QUnit.test 
TypeError: Cannot read property 'substring' of undefined
    at Locale.months (C:\Users\tim\Documents\GitHub\moment\build\umd\locale\el.js:21:28)
    at Object.<anonymous> 

Module: locale:hr Test: valid localeData
months should return 12 months
Actual value:undefined
Expected value:12

Module: locale:hy-am Test: valid localeData
months should return 12 months
Actual value:undefined
Expected value:12

Module: locale:ka Test: valid localeData
months should return 12 months
Actual value:undefined
Expected value:12

Module: locale:ka Test: valid localeData
weekdays should return 7 days
Actual value:undefined
Expected value:7

Module: locale:lt Test: valid localeData
months should return 12 months
Actual value:undefined
Expected value:12

Module: locale:lt Test: valid localeData
weekdays should return 7 days
Actual value:undefined
Expected value:7

Module: locale:pl Test: valid localeData
Died on test #1     at global.test.QUnit.test (C:\Users\tim\Documents\GitHub\moment\node_modules\qunit\lib\child.js:146:21)
...    at tryModuleLoad (module.js:446:12): Cannot read property 'month' of undefined
TypeError: Cannot read property 'month' of undefined

Module: locale:ru Test: valid localeData
months should return 12 months
Actual value:undefined
Expected value:12

Module: locale:ru Test: valid localeData
monthsShort should return 12 months
Actual value:undefined
Expected value:12

Module: locale:ru Test: valid localeData
weekdays should return 7 days
Actual value:undefined
Expected value:7

Module: locale:uk Test: valid localeData
months should return 12 months
Actual value:undefined
Expected value:12

Module: locale:uk Test: valid localeData
Died on test #3     at global.test.QUnit.test (C:\Users\tim\Documents\GitHub\moment\node_modules\qunit\lib\child.js:146:21)
...    at tryModuleLoad (module.js:446:12): Cannot read property 'day' of undefined

@timfish
Copy link
Contributor Author

timfish commented Mar 5, 2017

I've got all the tests passing but it might not be the ideal long term solution. At least now all locales return months and days.

All the changed locales will need checking to ensure I've returned the correct tenses/cases for the months/days.

month.js already contained:

export function localeMonths (m, format) {
    if (!m) {
        return this._months;
    }
    return isArray(this._months) ? this._months[m.month()] :
        this._months[(this._months.isFormat || MONTHS_IN_FORMAT).test(format) ? 'format' : 'standalone'][m.month()];
}

export function localeMonthsShort (m, format) {
    if (!m) {
        return this._monthsShort;
    }
    return isArray(this._monthsShort) ? this._monthsShort[m.month()] :
        this._monthsShort[MONTHS_IN_FORMAT.test(format) ? 'format' : 'standalone'][m.month()];
}

So if we could get the localeData loading through those it would have worked without changing the locales. It's certainly in need of some refactoring.

@ichernev
Copy link
Contributor

ichernev commented Mar 5, 2017

@timfish wait a sec. First -- why don't you just fix the generic function, not rewrite standalone/format the primitive way. src/lib/unit/month.js : localeMonths, localeMonthsShort, src/lib/unit/day-of-week.js : localeWeekdays, localeWeekdaysShort, localeWeekdaysMin.

Second -- if you need to do something on all locales, use src/test/helpers/common-locale.js.

@timfish
Copy link
Contributor Author

timfish commented Mar 5, 2017

Easy when you know how!

@ichernev ichernev added this to the 2.18.0 milestone Mar 5, 2017
@ichernev
Copy link
Contributor

ichernev commented Mar 5, 2017

@timfish thank you, looks great!

@ichernev
Copy link
Contributor

Merged in 1d70961

ichernev added a commit that referenced this pull request Mar 11, 2017
[bugfix] Fixes #3805, fix locale month getters for standalone/format cases
@ichernev ichernev changed the title Fix issue #3805 [bugfix] Fixes #3805, fix locale month getters for standalone/format cases Mar 11, 2017
@ichernev ichernev closed this Mar 11, 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

4 participants