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] updateLocale now tries to load parent, fixes #3626 #4242

Closed
wants to merge 3 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@cmyers
Contributor

cmyers commented Oct 14, 2017

This addresses #3626.

A fix was suggested in the comments but a PR wasn't submitted. I've refactored the suggested fix which will check if the locale to update exists (if not found in the currently loaded locales) and load it before merging. This mitigates the merge with the current set locale if the submitted locale isn't found in the loaded locales array.

Not sure if it should continue to merge if the locale to merge with can't be loaded, but this could be another issue to discuss.

@icambron

This comment has been minimized.

Show comment
Hide comment
@icambron

icambron Oct 19, 2017

Member

@cmyers Can you add a test?

Member

icambron commented Oct 19, 2017

@cmyers Can you add a test?

@cmyers

This comment has been minimized.

Show comment
Hide comment
@cmyers

cmyers Oct 19, 2017

Contributor

@icambron new to qunit but I'll give it a go.

Contributor

cmyers commented Oct 19, 2017

@icambron new to qunit but I'll give it a go.

@icambron

This comment has been minimized.

Show comment
Hide comment
@icambron

icambron Oct 19, 2017

Member

Actually, another thought: I looked at the loadLocale code and it looks like it checks the cache anyway. So you probably don't even need the locales[locale] || part. Just let loadLocale check for you.

Member

icambron commented Oct 19, 2017

Actually, another thought: I looked at the loadLocale code and it looks like it checks the cache anyway. So you probably don't even need the locales[locale] || part. Just let loadLocale check for you.

@cmyers

This comment has been minimized.

Show comment
Hide comment
@cmyers

cmyers Oct 19, 2017

Contributor

That's a good catch, I'll revisit then add a test.

Contributor

cmyers commented Oct 19, 2017

That's a good catch, I'll revisit then add a test.

@icambron

This comment has been minimized.

Show comment
Hide comment
@icambron

icambron Oct 20, 2017

Member

LGTM. Thanks!

Member

icambron commented Oct 20, 2017

LGTM. Thanks!

@ichernev

This comment has been minimized.

Show comment
Hide comment
@ichernev

ichernev Oct 24, 2017

Contributor

So basically try to load it if its not loaded already? That's mostly for node.js, I guess.

Contributor

ichernev commented Oct 24, 2017

So basically try to load it if its not loaded already? That's mostly for node.js, I guess.

@icambron

This comment has been minimized.

Show comment
Hide comment
@icambron

icambron Oct 25, 2017

Member

Yup

Member

icambron commented Oct 25, 2017

Yup

@ichernev

This comment has been minimized.

Show comment
Hide comment
@ichernev

ichernev Nov 11, 2017

Contributor

Merged in d26f97e

Contributor

ichernev commented Nov 11, 2017

Merged in d26f97e

@ichernev ichernev changed the title from Fix #3626: updateLocale merge issue to [bugfix] updateLocale now tries to load parent, fixes #3626 Nov 11, 2017

@ichernev ichernev closed this Nov 11, 2017

ichernev added a commit that referenced this pull request Nov 11, 2017

Merge pull request #4242 from cmyers:develop
[bugfix] updateLocale now tries to load parent, fixes #3626
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment