Skip to content

feat(isISO6391): add isISO6391() functionality #1103

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
Closed
ezkemboi wants to merge 1 commit into from
Closed

feat(isISO6391): add isISO6391() functionality #1103

ezkemboi wants to merge 1 commit into from

Conversation

@ezkemboi
Copy link
Member

@ezkemboi ezkemboi commented Sep 9, 2019

tux-tn
tux-tn reviewed Oct 14, 2019
View reviewed changes
Copy link
Member

@tux-tn tux-tn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

isISO6391('xx') will return true even if xx is not a valid ISO 639-1 code
Don't you think that having a regex with all possibilities is more accurate since ISO 639-1 is a standardized list?

@ezkemboi
Copy link
Member Author

ezkemboi commented Oct 14, 2019

@tux-tn, good suggestion. Let me check how long is the list of the codes.

@tux-tn
Copy link
Member

tux-tn commented Jan 20, 2020

@ezkemboi any progress? I needed an ISO639-1 validation regex today and found this gist

@ezkemboi
Copy link
Member Author

ezkemboi commented Jan 20, 2020

@tux-tn let me work on this today evening or tomorrow and update it.
Sorry for any delay caused.

@ezkemboi
Copy link
Member Author

ezkemboi commented Jan 20, 2020

this gist

I think this will help.
I will check out and use it.

Thanks.

@tux-tn
Copy link
Member

tux-tn commented Jan 20, 2020

@ezkemboi take your time! Thank you for working on this

@ezkemboi ezkemboi force-pushed the isISO6391 branch 2 times, most recently from 598d6bc to 9ef2dfb Compare January 20, 2020 18:06
@ezkemboi
Copy link
Member Author

ezkemboi commented Jan 20, 2020

@tux-tn, I have fixed the code, though after running the test, it has changed many more files.

@ezkemboi
Copy link
Member Author

ezkemboi commented Feb 8, 2020

Ping @profnandaa @tux-tn

profnandaa
profnandaa reviewed Feb 10, 2020
View reviewed changes
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.

See my minor suggestion, and also remove the unrelated changes.

src/lib/isISO6391.js Outdated
@@ -0,0 +1,8 @@
import assertString from './util/assertString';

const isISO6391Reg = /^(aa|ab|ae|af|ak|am|an|ar|as|av|ay|az|az|ba|be|bg|bh|bi|bm|bn|bo|br|bs|ca|ce|ch|co|cr|cs|cu|cv|cy|da|de|dv|dz|ee|el|en|eo|es|et|eu|fa|ff|fi|fj|fo|fr|fy|ga|gd|gl|gn|gu|gv|ha|he|hi|ho|hr|ht|hu|hy|hz|ia|id|ie|ig|ii|ik|io|is|it|iu|ja|jv|ka|kg|ki|kj|kk|kl|km|kn|ko|kr|ks|ku|kv|kw|ky|la|lb|lg|li|ln|lo|lt|lu|lv|mg|mh|mi|mk|ml|mn|mr|ms|mt|my|na|nb|nd|ne|ng|nl|nn|no|nr|nv|ny|oc|oj|om|or|os|pa|pi|pl|ps|pt|qu|rm|rn|ro|ru|rw|sa|sc|sd|se|sg|si|sk|sl|sm|sn|so|sq|sr|ss|st|su|sv|sw|ta|te|tg|th|ti|tk|tl|tn|to|tr|ts|tt|tw|ty|ug|uk|ur|uz|ve|vi|vo|wa|wo|xh|yi|yo|za|zh|zu)$/i;
Copy link
Member

@profnandaa profnandaa Feb 10, 2020

Choose a reason for hiding this comment

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

Instead of a regex, could we just use a set instead? Listing the items on each line for better readability:

const isoSet = new Set([
  'aa',
  'ab',
  //...
]);

and return isoSet.has(str).

Copy link
Member Author

@ezkemboi ezkemboi Feb 10, 2020

Choose a reason for hiding this comment

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

Alright, thanks, looking at making the requested changes.

@ezkemboi
Copy link
Member Author

ezkemboi commented Feb 10, 2020

@profnandaa I have tried to fix them.
Though, I don't know why some of the files still get generated when we merge upstream to our local branches?

@profnandaa
Copy link
Member

profnandaa commented Feb 11, 2020

@ezkemboi -- make sure you rebase with the latest validatorjs:master.

@profnandaa profnandaa added mc-to-land Just merge-conflict standing between the PR and landing. ready-to-land For PRs that are reviewed and ready to be landed and removed 🕑 to-be-reviewed labels Feb 14, 2020
@profnandaa
Copy link
Member

profnandaa commented Feb 15, 2020

@ezkemboi -- could you also check my comments here, let's check if there's any demand for this...

The idea here to try as much as possible (unless compelled by demand), to have the checking-against-db-ish sort of features, and strictly stick to regex.

I'd prefer that we put this PR on hold until a sizable number of vote is met; like what I saw with isLocale.

-na

@profnandaa profnandaa added needs-vote For edge-case feature requests that need popularity vote ⚠hold and removed mc-to-land Just merge-conflict standing between the PR and landing. ready-to-land For PRs that are reviewed and ready to be landed labels Feb 15, 2020
@ezkemboi
Copy link
Member Author

ezkemboi commented Feb 15, 2020

That is awesome @profnandaa.

@profnandaa profnandaa mentioned this pull request Feb 17, 2020
Closed
@profnandaa profnandaa closed this Mar 5, 2020
@braaar braaar mentioned this pull request Jan 4, 2022
Open
3 tasks
@braaar braaar mentioned this pull request Jan 4, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@profnandaa profnandaa profnandaa left review comments

@tux-tn tux-tn tux-tn left review comments

Assignees

No one assigned

Labels

⚠hold needs-vote For edge-case feature requests that need popularity vote

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add isISO6391() method

3 participants

@ezkemboi @tux-tn @profnandaa