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

feat(isLocale): validate isLocale #1072

Merged
merged 1 commit into from
Feb 15, 2020
Merged

Conversation

ezkemboi
Copy link
Member

  • It validates different languages for different geographical location
  • Closes isLocale #954

@profnandaa
Copy link
Member

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?

@ezkemboi
Copy link
Member Author

ezkemboi commented Aug 2, 2019

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.


/* eslint-disable max-len */
const locales = {
af_NA: 'Afrikaans (Namibia)',
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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.

@ezkemboi
Copy link
Member Author

@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.

@ezkemboi
Copy link
Member Author

Here is a link to different locales :
https://github.com/go-playground/locales

There are these two complex locales that I am trying to fix them on it also,
'en_US_POSIX',
'ca_ES_VALENCIA',

@ezkemboi
Copy link
Member Author

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 en_US_POSIX and ca_ES_VALENCIA, then it means that it might validate locales that do not exist or does not match the pattern.
E.g it can validate en_US_POSIXXX if let's say I check on character length of the last part.

So, this made me go with the option of adding if statement for the two.

@rubiin
Copy link
Member

rubiin commented Oct 15, 2019

@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') {
Copy link
Member

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? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they are exceptions.

@ezkemboi
Copy link
Member Author

@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 can help give point of view.

@ezkemboi
Copy link
Member Author

ezkemboi commented Oct 16, 2019

@profnandaa, I think @vlapo is in need of this feature?
But, from a library that makes use of validator.js
Here is a link
typestack/class-validator#425

@profnandaa
Copy link
Member

profnandaa commented Oct 17, 2019 via email

@ezkemboi
Copy link
Member Author

No problem @profnandaa.
cc. @vlapo.

@johannesschobel
Copy link
Contributor

what is the current status of this PR? thanks a lot!

@ezkemboi
Copy link
Member Author

@profnandaa can inform us if it is ready.
On my end it is good.

@johannesschobel
Copy link
Contributor

ping again .. i would really need this feature in class-validator..

@ezkemboi
Copy link
Member Author

ezkemboi commented Dec 4, 2019

@profnandaa and @chriso.
Will you be able to have time to check on this one, please?

@rubiin
Copy link
Member

rubiin commented Dec 4, 2019

@profnandaa and @chriso.
Will you be able to have time to check on this one, please?

This is a most awaited feature as some other things like language validation also kind of depends on this

@profnandaa
Copy link
Member

Sorry for my delay on this one, on it! 👍

Copy link
Member

@profnandaa profnandaa left a 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 profnandaa added needs-more-review 🧹 needs-update For PRs that need to be updated before landing labels Dec 6, 2019
@ezkemboi
Copy link
Member Author

ezkemboi commented Dec 6, 2019

@profnandaa, I have fixed the merge conflicts. Though there are some unnecessary changes here.
I will have a look at it. I think I did a merge, which has taken such a direction.

@rubiin, @johannesschobel can you have an extra eye on this one.
Thanks in advance.

@rubiin
Copy link
Member

rubiin commented Dec 6, 2019

@profnandaa, I have fixed the merge conflicts. Though there are some unnecessary changes here.
I will have a look at it. I think I did a merge, which has taken such a direction.

@rubiin, @johannesschobel can you have an extra eye on this one.
Thanks in advance.

sure

@rubiin
Copy link
Member

rubiin commented Dec 6, 2019

looks good so far

@johannesschobel
Copy link
Contributor

when can this be merged? 😟

@arkus7
Copy link

arkus7 commented Jan 24, 2020

Any update on this? 🙏

@ezkemboi
Copy link
Member Author

@profnandaa, I see that many people are demanding this feature.
Is there anything we can do to accelerate it's landing?

@profnandaa
Copy link
Member

@ezkemboi -- please fix the merge conflicts and I should land it.

@profnandaa
Copy link
Member

Remember to remove the unrelated changes, I don't think we should have a 17-files diff.

@ezkemboi
Copy link
Member Author

I will do that.

@ezkemboi
Copy link
Member Author

@profnandaa, I have fixed the issues raised.
You can take a look at it.

@profnandaa
Copy link
Member

1000 apologies for forgetting this PR.

@profnandaa profnandaa added ready-to-land For PRs that are reviewed and ready to be landed and removed needs-more-review labels Feb 14, 2020
@profnandaa
Copy link
Member

@ezkemboi -- just this m/c, I'll get a notification when you push. I'll merge it as soon.

@ezkemboi
Copy link
Member Author

ezkemboi commented Feb 14, 2020

@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
@profnandaa profnandaa merged commit 7bd816e into validatorjs:master Feb 15, 2020
@johannesschobel
Copy link
Contributor

Woooooooha.. Thanks a lot

@rubiin
Copy link
Member

rubiin commented Feb 15, 2020

Hope it gets published on next release

@johannesschobel
Copy link
Contributor

Dear @profnandaa ,
can you please tag this commit as a new version?! it has been merged, but not released!

thanks a lot

@profnandaa
Copy link
Member

@johannesschobel -- sure, our Feb release should be going out soon.
/cc. @chriso

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 needs-update For PRs that need to be updated before landing ready-to-land For PRs that are reviewed and ready to be landed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isLocale
5 participants