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

added support for IL in isIdentityCard() #1041

Merged
merged 7 commits into from
Jun 18, 2019
Merged

added support for IL in isIdentityCard() #1041

merged 7 commits into from
Jun 18, 2019

Conversation

perimiter
Copy link
Contributor

i added IL to readme and i used build for babel . i added tests like in ES . all changes i made are in lib not in src(although when i look in the changed files i can see the changes in src as well,is that bables doing? ).
i hope this time i did it correctly .

@ezkemboi
Copy link
Member

@perimiter don't worry about the changes that reflected on the src. That should happen and @profnandaa can give more reference on the same here.
Also, make sure you run tests npm test and make sure they pass locally before pushing your changes.

@perimiter
Copy link
Contributor Author

thank you. i figured out the test thing after a few commits. i have a technical question though , i used a test that is valid on ES as invalid for me but that failed . i took tests that are invalid for ES and now everything works. why is that?

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.

Thanks for the PR! 🎉

LGTM, just fix the merge conflicts and we should be ready to land.

@profnandaa profnandaa added 🧹 needs-update For PRs that need to be updated before landing and removed 🕑 to-be-reviewed labels Jun 18, 2019
@perimiter
Copy link
Contributor Author

perimiter commented Jun 18, 2019

@profnandaa how can i merge the conflicts using an IDE instead of the editor ? files like validator.min.js are build by babel.

@profnandaa
Copy link
Member

@perimiter -- I see, only fix the merge conflicts on README, src and test then re-build (run the tests again). Once all is ok, make and new commit and push.

@profnandaa profnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Jun 18, 2019
README.md Outdated
@@ -100,7 +100,7 @@ Validator | Description
**isHash(str, algorithm)** | check if the string is a hash of type algorithm.<br/><br/>Algorithm is one of `['md4', 'md5', 'sha1', 'sha256', 'sha384', 'sha512', 'ripemd128', 'ripemd160', 'tiger128', 'tiger160', 'tiger192', 'crc32', 'crc32b']`
**isHexColor(str)** | check if the string is a hexadecimal color.
**isHexadecimal(str)** | check if the string is a hexadecimal number.
**isIdentityCard(str [, locale])** | check if the string is a valid identity card code.<br/><br/>`locale` is one of `['ES', 'zh-TW']` OR `'any'`. If 'any' is used, function will check if any of the locals match.<br/><br/>Defaults to 'any'.
**isIdentityCard(str [, locale])** | check if the string is a valid identity card code.<br/><br/>`locale` is one of `['ES', 'zh-TW', 'IL']` OR `'any'`. If 'any' is used, function will check if any of the locals match.<br/><br/>Defaults to 'any'.
Copy link
Member

Choose a reason for hiding this comment

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

@perimiter - we should be good now. However, I missed out this one. Please change the locale to he-IL here and in the other files, then we should be good to go.

Copy link
Member

Choose a reason for hiding this comment

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

As used in isMobilePhone and also see here for ref

@profnandaa profnandaa self-requested a review June 18, 2019 15:38
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.

Update the locale ID.


const id = sanitized;

if (id.length !== 9 || isNaN(id)) {
Copy link
Member

Choose a reason for hiding this comment

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

@profnandaa isn't this check unnecessary if we add the size limit in the regex?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that's true. That was oversight from my end. You can do a quick update PR on this?

const DNI = /^\d{9}$/;

cc. @perimiter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats true, but you still need the if statement for the isNaN()

Copy link
Member

@tux-tn tux-tn Jun 18, 2019

Choose a reason for hiding this comment

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

@perimiter your regex is searching only for digits, the function will always exit before the call to isNaN if it's not a number.
As you can see, the path is never taken in the tests
Capture d’écran

Copy link
Contributor Author

@perimiter perimiter Jun 18, 2019

Choose a reason for hiding this comment

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

@tux-tn your right, i opened a PR with the new changes (#1048)

@profnandaa profnandaa merged commit 7735e0f into validatorjs:master Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants