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] locales: Enable locale-data getters without moment (fixes #3284) #3312

Closed
wants to merge 3 commits into from
Closed

Conversation

theawesomenayak
Copy link
Contributor

This possibly fixes the issue #3284. Can you please verify it @icambron ?

In the absence of a moment parameter, it returns a locale specific list.

@@ -99,17 +99,26 @@ function parseIsoWeekday(input, locale) {

export var defaultLocaleWeekdays = 'Sunday_Monday_Tuesday_Wednesday_Thursday_Friday_Saturday'.split('_');
export function localeWeekdays (m, format) {
if (typeof m === 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

There's a utils.isUndefined. I forget what advantage it has, but we should be consistent.

@icambron
Copy link
Member

This seems like a reasonable approach to me. @ichernev, what do you think?

return this._weekdaysShort[m.day()];
}

export var defaultLocaleWeekdaysMin = 'Su_Mo_Tu_We_Th_Fr_Sa'.split('_');
export function localeWeekdaysMin (m) {
if (typeof m === 'undefined') {
return this._weekdaysMin;
}
return this._weekdaysMin[m.day()];
}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this code be cleaner as:

      return (m) ? this._weekdaysMin[m.day()] : this._weekdaysMin;

Also, this pull needs unit tests.

@theawesomenayak
Copy link
Contributor Author

I have added unit tests and cleaned up the code.

@@ -12,6 +12,7 @@ import isArray from '../utils/is-array';
import indexOf from '../utils/index-of';
import { createUTC } from '../create/utc';
import getParsingFlags from '../create/parsing-flags';
import isUndefined from '../utils/is-undefined';

Copy link
Member

Choose a reason for hiding this comment

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

is this util being used? Note that my suggestion changed the meaning of the code. Previously you were checking for undefined, and now you're checking for falsiness. I think they achieve what amounts to the same thing, but I want to point out that semantic change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, my bad. I forgot to remove that import. Will do that.
So should I use typeof m === 'undefined' or !m?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, now you can drop the line :)

@ichernev
Copy link
Contributor

Can you point to the place in the docs that this behavior is documented?
http://momentjs.com/docs/#/i18n/locale-data/ here its listed that you do need to pass a moment object. I'm not saying the change is bad, I'm just trying to understand (and leave it documented here).

@ichernev
Copy link
Contributor

ichernev commented Sep 1, 2016

Merged in 626bd19

@ichernev ichernev changed the title Fix for locale-data without moment (fixes #3284) [bugfix] locales: Enable locale-datnwithout moment ta getters without moment (fixes #3284) Sep 1, 2016
@ichernev ichernev closed this Sep 1, 2016
ichernev added a commit that referenced this pull request Sep 1, 2016
[bugfix] locales: Enable locale-datnwithout moment ta getters without moment (fixes #3284)
@ichernev ichernev changed the title [bugfix] locales: Enable locale-datnwithout moment ta getters without moment (fixes #3284) [bugfix] locales: Enable locale-data getters without moment (fixes #3284) Sep 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants