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.

Sign up for GitHub

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

Jump to bottom

[new locale] nl-be: Dutch (Belgium) locale #3416

Closed
wants to merge 3 commits into from
Closed

[new locale] nl-be: Dutch (Belgium) locale #3416

wants to merge 3 commits into from

Conversation

kevinzwhuang
Copy link
Contributor

These changes address issue #3410:

  • Renames nl locale to nl-nl for Dutch (Netherlands) locale
  • Creates nl-be for Dutch (Belgium) locale

References for nl-be locale:

References for nl-nl locale:

How does this look to you @jorisroling, @middagj?

@middagj
Copy link
Contributor

middagj commented Sep 5, 2016

I am Dutch (as in nl-nl), so I can't really comment on the standard Belgian way of writing dates. I do see it sometimes like that, and the linked sources do confirm. Is it possible to have a common ancestor of the two files?

(Sorry about the line comment, only if the month is numeric slashes are used so no need to change the if statement.)

@kevinzwhuang
Copy link
Contributor Author

I just added a commit to restore the original src/locale/nl.js & src/test/locale/nl.js to serve as a common ancestor for both nl-nl & nl-be, does that work @middagj?

@middagj
Copy link
Contributor

middagj commented Sep 5, 2016

Oh that is also nice, for if you don't care about NL or BE. I was actually more referring to duplication in the code. I don't know if that is easily done in the current framework.

@kevinzwhuang
Copy link
Contributor Author

Yeah, duplicating it as NL for now is probably the way to go. AR and the
other AR locales ( AR-LY, AR-MA, ...etc ) also follow this pattern ( see
ar.js, ar-ly.js, etc. )

This is nice too since anyone using NL currently will not suffer a breaking
change from losing the locale to NL-NL.

On Mon, Sep 5, 2016, 11:54 AM Jacob Middag notifications@github.com wrote:

Oh that is also nice, for if you don't care about NL or BE. I was actually
more referring to duplication in the code. I don't know if that is easily
done in the current framework.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#3416 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AG64ch7Qe_rOAsSPpm7TgbQ0Fr32V1cgks5qnGV9gaJpZM4JzdtT
.

@kevinzwhuang
Copy link
Contributor Author

Revisiting this PR after a while, @mj1856 how does this look to you?

@mattjohnsonpint
Copy link
Contributor

You don't need to do anything with nl, nor do you need a nl-nl copy. Just add the one file nl-be.js (and its tests).

Consider that moment.locale('nl-anything') will currently use nl.

@kevinzwhuang
Copy link
Contributor Author

kevinzwhuang commented Sep 14, 2016 •
edited
Loading

Thanks @mj1856 , I went ahead and removed nl-nl. Now the only addition will be nl-be and its tests

@kevinzwhuang
Copy link
Contributor Author

Does this PR look good to you @mj1856 ?

@ichernev
Copy link
Contributor

ichernev commented Nov 1, 2016

@kevinzwhuang code looks great. You'd also need to sing the CLA (a new thing). This will go out in next release. Thanks for contributing!

@kevinzwhuang
Copy link
Contributor Author

Thanks @ichernev, just signed the CLA!

@kevinzwhuang kevinzwhuang changed the title Create nl-be (Belgium) locale & rename nl (Netherlands) to nl-nl Create nl-be (Belgium) locale Nov 1, 2016
@ichernev
Copy link
Contributor

ichernev commented Nov 6, 2016

Merged in 0ad3055

@ichernev ichernev changed the title Create nl-be (Belgium) locale [new locale] nl-be: Dutch (Belgium) locale Nov 6, 2016
@ichernev ichernev closed this Nov 6, 2016
ichernev added a commit that referenced this pull request Nov 6, 2016
@ichernev
Merge pull request #3416 from kevinzwhuang:migrate-nl-locales
0ad3055 
[new locale] nl-be: Dutch (Belgium) locale
@mattjohnsonpint mattjohnsonpint added this to the 2.16.0 milestone Nov 10, 2016
@icambron icambron mentioned this pull request Nov 25, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Language
Projects
None yet
Milestone
2.16.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kevinzwhuang @middagj @mattjohnsonpint @ichernev