-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat(isLocale): validate isLocale #1072
Conversation
ezkemboi
commented
Jul 28, 2019
- It validates different languages for different geographical location
- Closes isLocale #954
I'll need some more time to look into this. There's also a general ISO guideline for locales, perhaps we should also consider that. QQ, what was your source for this locales, can include the ref link on the PR? |
Here is what I used @profnandaa https://gist.github.com/jacobbubu/1836273 But, looking at this, https://gist.github.com/jasef/337431c43c3addb2cbd5eb215b376179, https://gist.github.com/jacobbubu/1836273 and https://github.com/umpirsky/locale-list/tree/master/data, there seem to be more locales available. But, I am fetching those locale data are the ones provided by Apple devices. |
src/lib/isLocale.js
Outdated
|
||
/* eslint-disable max-len */ | ||
const locales = { | ||
af_NA: 'Afrikaans (Namibia)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible to come up with a general regex for this, since we're bound to have more locales coming up. I see for instance en_KE
is left out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, no problem. I can come up with it, though it might not be a guarantee that we will get them. (In case I find a unique way of identifying a locale, sure, will do the regex).
Alternatively, let me try find as many possible locales as possible for the same. And add them. I will try use some more resources such as this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@profnandaa, I think after reading this StackOverflow discussion,
I will go ahead to make use of regex. Thanks for your thoughts.
What locale could consists of:
1. Language code: ISO 639 2 or 3, or 4 for future use, alpha.
2. Optional script code: ISO 15924 4 alpha or 3 digit.
3. Optional country code: ISO 3166-1 2 alpha or 3 digit.
4. Separated by underscores or dashes.
Valid examples are:
- de
- en-US
- zh-Hant-TW
- En-au
- aZ_cYrl-aZ.
You can add label needs update
on this one.
221e743
to
f41ac00
Compare
@profnandaa, I have updated the PR to make use of regex instead of listing all possible locales, which keep on increasing and at the same time they are too many. |
Here is a link to different locales : There are these two complex locales that I am trying to fix them on it also, |
Since the regex validates all other locales except the two mentioned above, I have added this code if (str === 'en_US_POSIX' || str === 'ca_ES_VALENCIA') {
return true;
} If I add regex to validate So, this made me go with the option of adding if statement for the two. |
@profnandaa @ezkemboi I think we can use this to check language too |
|
||
export default function isLocale(str) { | ||
assertString(str); | ||
if (str === 'en_US_POSIX' || str === 'ca_ES_VALENCIA') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these 2 the only exceptions? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they are exceptions.
@rubiin, thanks for the feedback, I will do a check on that possibility (Though, I thought languages are more than what are in locales). |
@profnandaa, I think @vlapo is in need of this feature? |
Sorry for the delay. Will check and land today, Though the release will
take some time.
./na
On Wed, Oct 16, 2019 at 11:06 PM Ezrqn Kemboi ***@***.***> wrote:
@profnandaa <https://github.com/profnandaa>, I think @vlapo
<https://github.com/vlapo> is in need of this feature?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1072?email_source=notifications&email_token=AAB7ZEINMIYGZVBQ35D3S6TQO5X6HA5CNFSM4IHMMFA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBNYSBI#issuecomment-542869765>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB7ZEML3Z7RSXH4PDAK3STQO5X6HANCNFSM4IHMMFAQ>
.
--
Sent from a tiny device while on the move.
|
No problem @profnandaa. |
what is the current status of this PR? thanks a lot! |
@profnandaa can inform us if it is ready. |
ping again .. i would really need this feature in class-validator.. |
@profnandaa and @chriso. |
This is a most awaited feature as some other things like language validation also kind of depends on this |
Sorry for my delay on this one, on it! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make use of any extra pair of eyes. LGTM though, now just fix the merge conflicts.
@profnandaa, I have fixed the merge conflicts. Though there are some unnecessary changes here. @rubiin, @johannesschobel can you have an extra eye on this one. |
sure |
looks good so far |
when can this be merged? 😟 |
Any update on this? 🙏 |
@profnandaa, I see that many people are demanding this feature. |
@ezkemboi -- please fix the merge conflicts and I should land it. |
Remember to remove the unrelated changes, I don't think we should have a 17-files diff. |
I will do that. |
@profnandaa, I have fixed the issues raised. |
1000 apologies for forgetting this PR. |
@ezkemboi -- just this m/c, I'll get a notification when you push. I'll merge it as soon. |
@profnandaa on it. Let me finish some sort of issue here coz, it seemed I had earlier merged the changes rather than rebasing them. |
- it validates different languages for different geographical location - Closes validatorjs#954
Woooooooha.. Thanks a lot |
Hope it gets published on next release |
Dear @profnandaa , thanks a lot |
@johannesschobel -- sure, our Feb release should be going out soon. |