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(isLicensePlate): adding license plate validators for 'mercosur' and 'pt-BR' locales #1588

Merged
merged 2 commits into from
Mar 12, 2021

Conversation

renanmontebelo
Copy link
Contributor

@renanmontebelo renanmontebelo commented Jan 31, 2021

Update 2021-03-08: removing mercosur as per the discussion.

Adding 2 new validators for license plates:

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Jan 31, 2021

Codecov Report

Merging #1588 (fb6575b) into master (1b85829) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1588   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         1796      1808   +12     
=========================================
+ Hits          1796      1808   +12     
Impacted Files Coverage Δ
src/lib/isPassportNumber.js 100.00% <ø> (ø)
src/lib/isAlphanumeric.js 100.00% <100.00%> (ø)
src/lib/isDataURI.js 100.00% <100.00%> (ø)
src/lib/isEAN.js 100.00% <100.00%> (ø)
src/lib/isIdentityCard.js 100.00% <100.00%> (ø)
src/lib/isLicensePlate.js 100.00% <100.00%> (ø)
src/lib/isMACAddress.js 100.00% <100.00%> (ø)
src/lib/isSlug.js 100.00% <100.00%> (ø)
src/lib/isStrongPassword.js 100.00% <100.00%> (ø)
src/lib/rtrim.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b85829...4f0dc0e. Read the comment docs.

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.

Sounds good @renanmontebelo

A little suggestion, since we are using country codes for most of the validators including isLicensePlate. Can you replace mercosur with corresponding countries ? (Someone validating a license plate in Argentina should expect to use the country code instead of mercosur)

Something like:

const mercosurCountries  = ['es-AR', 'es-PY', 'es-UY'];
const mercosur = str =>
    /^[A-Z]{2}[ -]?[0-9]{3}[ -]?[A-Z]{2}|[A-Z]{3}[ -]?[0-9][A-Z][0-9]{2}|[A-Z]{4}[ -]?[0-9]{3}|[A-Z]{3}[ -]?[0-9]{4}$/.test(str);
for (let i = 0; i < mercosurCountries.length; i++) {
  validators[mercosurCountries[i]] = mercosur;
}

And i guess since Brasil is supporting two types of license plate you can add the mercosur validation to the existing PT-BR regex.
@profnandaa any feedback on this? Should we add a new mercusor option?

@renanmontebelo
Copy link
Contributor Author

@tux-tn that's a good point, I was wondering myself if including these rules as mercosur would be a good idea. It's not an easy decision: it's an international agreement, but countries have their own particularities on top of it. For instance, I know for a fact that Brazil is still in transition from old to new format, so both formats are actually valid. But older format is not a valid mercosur license. So my intention was to keep mercosur rules only for new formats while country-specific rules would be for any differences or transition periods.

So if we're removing mercosur because it's not a valid country locale then I'm OK with keeping only pt-br format; someone else more familiar with other countries might include their own regexes. I believe these would strike the best balance between all the tradeoffs I mentioned.

So I pushed a new commit removing mercosur and keeping only pt-br. Thank you for your directions!

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.

@renanmontebelo I also think it will be easier if someone else more familiar with the subject make the changes since the other countries have probably another license plate format before the agreement.
Thank you again for your contribution 🎉 I think the actual version can be merged as it is without issues but let's wait for Anthony and other people feedback about adding mercosur as a locale.

@profnandaa
Copy link
Member

@renanmontebelo -- I see you already removed mercosur; however you make a good point on this, I'd suggest that you open an issue so that we keep discussing on the best way forward. As for now, I'll merge this one, LGTM. Thanks for your contribution! 🎉

@profnandaa profnandaa merged commit 7989e5b into validatorjs:master Mar 12, 2021
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