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(isEAN): implement isEAN validator #1244

Merged
merged 3 commits into from
Feb 5, 2020

Conversation

hamzahejja
Copy link
Contributor

@hamzahejja hamzahejja commented Feb 4, 2020

EAN: European Article Number.
Wikipedia: International Article Number

  • Per issue: Add support for validating EAN/GTIN barcodes #797 , this PR aims to integrate the isEAN(str) string validator 🎉
  • Supports validating string(s) against both EAN-8 and EAN-13.
  • Added tests to cover implementation for good/bad scenarios ✅

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 this. LGTM! 🎉

@profnandaa
Copy link
Member

@tux-tn @ezkemboi -- any extra review?

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

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

Looks great. Good job @hamzahejja! Can you take care of the 2 minor issues

src/lib/isEAN.js Outdated
* with exact numberic matching of 8 or 13 digits [0-9]
*/
const LENGTH_EAN_8 = 8;
const validEanRegex = /(?<!\d)(\d{8}|\d{13})(?!\d)$/;
Copy link
Member

Choose a reason for hiding this comment

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

RegExp lookbehind assertions are an ES2018 feature. They are not supported in all platforms.
By the way, can't a much simpler regex like /^(\d{8}|\d{13})$/ do the trick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tux-tn Yeah you're right, the /^(\d{8}|\d{13})$/ is sufficient to detect a consecutive sequence of 8-digits or 13-digits only ... Forgot about the lookbehind assertions being an ES2018 feature. Thanks! I've just modified it 👍


const remainder = 10 - (checksum % 10);

return remainder < 10 ? remainder : 0;
Copy link
Member

Choose a reason for hiding this comment

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

The case where remainder equals 10 is uncovered in mocha tests. Can you add a test case?

@hamzahejja
Copy link
Contributor Author

@tux-tn Changes addressed accordingly 👍

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

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

Thank you for your responsiveness Hamza. LGTM 🎉 !

@profnandaa profnandaa merged commit fc253c4 into validatorjs:master Feb 5, 2020
@hamzahejja hamzahejja deleted the add-isEAN-validator branch February 5, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants