-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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(pt-BR): tax id, passport and license plates #1613
Conversation
There's some overlap with these already opened PRs:
We might consider to join forces if you wish. |
Sure, for what I see @renanmontebelo license plate approach is more complete as "mercosur" plates are available too. My approach for tax Id validation uses the current I would suggest to keep his for license plates and mine for tax Ids. |
There was a problem hiding this 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 contribution @mschunke. Can you please resolve merge conflicts (delete validator.js
and validator.min.js
files and fix conflict in README.md
) and that would be great if you join forces and decide what to keep in each PR.
MB I approved the PR instead of requesting changes 😅
Codecov Report
@@ Coverage Diff @@
## master #1613 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 100 100
Lines 1807 1849 +42
=========================================
+ Hits 1807 1849 +42
Continue to review full report at Codecov.
|
Hey @tux-tn, all conflicts are solved and build checks passed. Anything else I should do? |
tin === '88888888888' || | ||
tin === '99999999999' || | ||
tin === '00000000000' | ||
) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not covered by tests, you probably just need to add a testcase with an invalid CPF
tin === '66666666666666' || | ||
tin === '77777777777777' || | ||
tin === '88888888888888' || | ||
tin === '99999999999999') { return false; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, this condition is not covered, adding a test with an invalid CNPJ should fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?
@renanmontebelo @mschunke what have you decided concerning what approach to keep? |
Hey, @tux-tn, |
@mschunke sorry for the delay, I can only work on this in the weekends. Selecting your PR for tax id and my PR for license plates sounds good. I'll close #1589 in favor of yours. My tax id implementation also accepts an optional parameter to require or ignore formatting characters like |
@renanmontebelo np. We all work on these on our spare time. I've removed my license plate validation for we can use yours, and kept my tax ID validator as agreed. Thanks for the cooperation! btw, my tax id validator accepts both formatted and unformatted input. |
Sounds good, thank you for the cooperation as well. I missed that yours accept both formatted/unformatted CPFs, that's awesome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @mschunke 🎉 ! Can you just remove the isLicensePlate
PT-BR entry in README.md
? i think it's the only necessary change
@tux-tn sorry about that, forget about the readme file! File updated. Please let me know if you need anything else 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mschunke that was fast 😅
LGTM 🎉 Thank you again for the fast response and for your collaboration with @renanmontebelo !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mschunke for the PR and welcome to the project! 🎉
Thanks @profnandaa! Happy to help :) |
Included Tax ID, passport, and license plate validators for
pt-BR
(Brazil) locale. Tests written and passed. Locale added to the documentation.Checklist