feat(pt-BR): tax id, passport and license plates - #1613

Merged
profnandaa merged 6 commits into
validatorjs:masterfrom
mschunke:master
Mar 12, 2021
Merged

feat(pt-BR): tax id, passport and license plates#1613
profnandaa merged 6 commits into
validatorjs:masterfrom
mschunke:master

Conversation

@mschunke

@mschunkemschunke commented Feb 21, 2021

Copy link
Copy Markdown
Contributor

Included Tax ID, passport, and license plate validators for pt-BR (Brazil) locale. Tests written and passed. Locale added to the documentation.

Checklist

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

@renanmontebelo

renanmontebelo commented Feb 28, 2021

Copy link
Copy Markdown
Contributor

There's some overlap with these already opened PRs:

We might consider to join forces if you wish.

@mschunke

mschunke commented Mar 1, 2021

Copy link
Copy Markdown
ContributorAuthor

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 .isTaxId function and supports also id (CNPJ) validation for entities and well as CPF (persons) validations.

I would suggest to keep his for license plates and mine for tax Ids.

tux-tn
tux-tn approved these changes Mar 1, 2021•
edited
Loading
View reviewed changes
tux-tn
tux-tn requested changes Mar 1, 2021
View reviewed changes

@tux-tntux-tn left a comment

Copy link
Copy Markdown
Member

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 😅

@tux-tntux-tn added 🍿 discussion 🧹 needs-update For PRs that need to be updated before landing labels Mar 1, 2021
@codecov

codecovBot commented Mar 5, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1613 (410ee54) into master (deb1d1e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #1613 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 100 100 Lines 1807 1849 +42 =========================================
+ Hits 1807 1849 +42
Impacted FilesCoverage Δ
src/lib/isPassportNumber.js100.00% <ø> (ø)
src/lib/isTaxID.js100.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 deb1d1e...a511310. Read the comment docs.

@mschunke

mschunke commented Mar 5, 2021

Copy link
Copy Markdown
ContributorAuthor

Hey @tux-tn, all conflicts are solved and build checks passed. Anything else I should do?

tux-tn
tux-tn reviewed Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
tin === '88888888888' ||
tin === '99999999999' ||
tin === '00000000000'
) return false;

@tux-tntux-tnMar 5, 2021

Copy link
Copy Markdown
Member

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

tux-tn
tux-tn reviewed Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
tin === '66666666666666' ||
tin === '77777777777777' ||
tin === '88888888888888' ||
tin === '99999999999999') { return false; }

@tux-tntux-tnMar 5, 2021

Copy link
Copy Markdown
Member

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

tux-tn
tux-tn requested changes Mar 5, 2021
View reviewed changes

@tux-tntux-tn left a comment

Copy link
Copy Markdown
Member

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?

@tux-tn

tux-tn commented Mar 5, 2021

Copy link
Copy Markdown
Member

@renanmontebelo@mschunke what have you decided concerning what approach to keep?
Should this one add only tax id and passport and the other one add license plate validation?

@mschunke

mschunke commented Mar 5, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@renanmontebelo@mschunke what have you decided concerning what approach to keep?
Should this one add only tax id and passport and the other one add license plate validation?

Hey, @tux-tn,
Got no reply from @renanmontebelo, but I'm fine with this PR just adding tax id and passport and using the other PR for license plates.

@mschunke

mschunke commented Mar 5, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?

@tux-tn
Was wondering where those untested lines were... thank you! Code updated with 100% coverage.

@renanmontebelo

renanmontebelo commented Mar 6, 2021

Copy link
Copy Markdown
Contributor

@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 . and -. Please feel free to copy into yours if you think it's a good addition. Thank you.

@renanmontebelorenanmontebelo mentioned this pull request Mar 6, 2021
Closed
3 tasks
@mschunke

mschunke commented Mar 6, 2021

Copy link
Copy Markdown
ContributorAuthor

@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.

@mschunke
mschunke requested a review from tux-tnMarch 6, 2021 19:49
@renanmontebelo

renanmontebelo commented Mar 6, 2021

Copy link
Copy Markdown
Contributor

Sounds good, thank you for the cooperation as well. I missed that yours accept both formatted/unformatted CPFs, that's awesome.

tux-tn
tux-tn requested changes Mar 8, 2021
View reviewed changes

@tux-tntux-tn left a comment

Copy link
Copy Markdown
Member

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

@mschunke

mschunke commented Mar 8, 2021

Copy link
Copy Markdown
ContributorAuthor

@tux-tn sorry about that, forget about the readme file! File updated. Please let me know if you need anything else 😁

tux-tn
tux-tn approved these changes Mar 8, 2021
View reviewed changes

@tux-tntux-tn left a comment

Copy link
Copy Markdown
Member

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 !

@tux-tntux-tn removed 🍿 discussion 🧹 needs-update For PRs that need to be updated before landing labels Mar 8, 2021
profnandaa
profnandaa approved these changes Mar 12, 2021
View reviewed changes

@profnandaaprofnandaa left a comment

Copy link
Copy Markdown
Member

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! 🎉

@profnandaa
profnandaa merged commit 3c771e8 into validatorjs:masterMar 12, 2021
@murilopulley

murilopulley commented Mar 12, 2021

Copy link
Copy Markdown

Thanks @profnandaa! Happy to help :)

@profnandaaprofnandaa mentioned this pull request Apr 19, 2021
Closed
@tux-tntux-tn mentioned this pull request Oct 4, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@profnandaaprofnandaaprofnandaa approved these changes
@tux-tntux-tntux-tn approved these changes

Assignees

No one assigned

Labels

🎉 first-prneeds-more-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mschunke@renanmontebelo@tux-tn@murilopulley@profnandaa
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
feat(pt-BR): tax id, passport and license plates - #1613
Merged
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Mar 12, 2021
Merged
feat(pt-BR): tax id, passport and license plates#1613
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Conversation
@mschunke
@mschunkemschunke
commented
Feb 21, 2021

Copy link


Copy Markdown

Contributor
Included Tax ID, passport, and license plate validators for pt-BR (Brazil) locale. Tests written and passed. Locale added to the documentation.
Checklist
  • [ X ] PR contains only changes related; no stray files, etc.
  • [ X ] README updated (where applicable)
  • [ X ] Tests written (where applicable)
@renanmontebelo
renanmontebelo
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
There's some overlap with these already opened PRs:
We might consider to join forces if you wish.
@mschunke
mschunke
commented
Mar 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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 .isTaxId function and supports also id (CNPJ) validation for entities and well as CPF (persons) validations.
I would suggest to keep his for license plates and mine for tax Ids.
tux-tn
tux-tn
approved these changes
Mar 1, 2021•

edited

Loading
View reviewed changes
tux-tn
tux-tn
requested changes
Mar 1, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 😅
@tux-tntux-tn
added

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 1, 2021
@codecov
codecovBot
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1613 (410ee54) into master (deb1d1e) will not change coverage.

The diff coverage is 100.00%.
Impacted file tree graph
@@ Coverage Diff @@## master #1613 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 100 100 Lines 1807 1849 +42 =========================================
+ Hits 1807 1849 +42 
Impacted FilesCoverage Δ
src/lib/isPassportNumber.js100.00% <ø> (ø)
src/lib/isTaxID.js100.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 deb1d1e...a511310. Read the comment docs.
@mschunke
mschunke
commented
Mar 5, 2021

Copy link


Copy Markdown

ContributorAuthor
Hey @tux-tn, all conflicts are solved and build checks passed. Anything else I should do?
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '88888888888' ||
 tin === '99999999999' ||
 tin === '00000000000'
 ) return false;
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '66666666666666' ||
 tin === '77777777777777' ||
 tin === '88888888888888' ||
 tin === '99999999999999') { return false; }
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
requested changes
Mar 5, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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?
@tux-tn
tux-tn
commented
Mar 5, 2021

Copy link


Copy Markdown

Member
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
Hey, @tux-tn,

Got no reply from @renanmontebelo, but I'm fine with this PR just adding tax id and passport and using the other PR for license plates.
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?
@tux-tn

Was wondering where those untested lines were... thank you! Code updated with 100% coverage.
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
@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 . and -. Please feel free to copy into yours if you think it's a good addition. Thank you.
@renanmontebelorenanmontebelo
mentioned this pull request
Mar 6, 2021
 Closed

3 tasks
@mschunke
mschunke
commented
Mar 6, 2021

Copy link


Copy Markdown

ContributorAuthor
@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.
@mschunke
mschunke
requested a review
from tux-tnMarch 6, 2021 19:49
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
Sounds good, thank you for the cooperation as well. I missed that yours accept both formatted/unformatted CPFs, that's awesome.
tux-tn
tux-tn
requested changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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
@mschunke
mschunke
commented
Mar 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@tux-tn sorry about that, forget about the readme file! File updated. Please let me know if you need anything else 😁
tux-tn
tux-tn
approved these changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 !
@tux-tntux-tn
removed

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 8, 2021
profnandaa
profnandaa
approved these changes
Mar 12, 2021
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
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! 🎉
@profnandaa
profnandaa
merged commit 3c771e8
into
validatorjs:masterMar 12, 2021
@murilopulley
murilopulley
commented
Mar 12, 2021

Copy link


Copy Markdown

Thanks @profnandaa! Happy to help :)
@profnandaaprofnandaa
mentioned this pull request
Apr 19, 2021
 Closed

@tux-tntux-tn
mentioned this pull request
Oct 4, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@profnandaaprofnandaaprofnandaa approved these changes
@tux-tntux-tntux-tn approved these changes

Assignees


No one assigned


Labels

🎉 first-prneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@mschunke@renanmontebelo@tux-tn@murilopulley@profnandaa

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(pt-BR): tax id, passport and license plates - #1613
Merged
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Mar 12, 2021
Merged
feat(pt-BR): tax id, passport and license plates#1613
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Conversation
@mschunke
@mschunkemschunke
commented
Feb 21, 2021

Copy link


Copy Markdown

Contributor
Included Tax ID, passport, and license plate validators for pt-BR (Brazil) locale. Tests written and passed. Locale added to the documentation.
Checklist
  • [ X ] PR contains only changes related; no stray files, etc.
  • [ X ] README updated (where applicable)
  • [ X ] Tests written (where applicable)
@renanmontebelo
renanmontebelo
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
There's some overlap with these already opened PRs:
We might consider to join forces if you wish.
@mschunke
mschunke
commented
Mar 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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 .isTaxId function and supports also id (CNPJ) validation for entities and well as CPF (persons) validations.
I would suggest to keep his for license plates and mine for tax Ids.
tux-tn
tux-tn
approved these changes
Mar 1, 2021•

edited

Loading
View reviewed changes
tux-tn
tux-tn
requested changes
Mar 1, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 😅
@tux-tntux-tn
added

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 1, 2021
@codecov
codecovBot
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1613 (410ee54) into master (deb1d1e) will not change coverage.

The diff coverage is 100.00%.
Impacted file tree graph
@@ Coverage Diff @@## master #1613 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 100 100 Lines 1807 1849 +42 =========================================
+ Hits 1807 1849 +42 
Impacted FilesCoverage Δ
src/lib/isPassportNumber.js100.00% <ø> (ø)
src/lib/isTaxID.js100.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 deb1d1e...a511310. Read the comment docs.
@mschunke
mschunke
commented
Mar 5, 2021

Copy link


Copy Markdown

ContributorAuthor
Hey @tux-tn, all conflicts are solved and build checks passed. Anything else I should do?
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '88888888888' ||
 tin === '99999999999' ||
 tin === '00000000000'
 ) return false;
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '66666666666666' ||
 tin === '77777777777777' ||
 tin === '88888888888888' ||
 tin === '99999999999999') { return false; }
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
requested changes
Mar 5, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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?
@tux-tn
tux-tn
commented
Mar 5, 2021

Copy link


Copy Markdown

Member
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
Hey, @tux-tn,

Got no reply from @renanmontebelo, but I'm fine with this PR just adding tax id and passport and using the other PR for license plates.
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?
@tux-tn

Was wondering where those untested lines were... thank you! Code updated with 100% coverage.
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
@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 . and -. Please feel free to copy into yours if you think it's a good addition. Thank you.
@renanmontebelorenanmontebelo
mentioned this pull request
Mar 6, 2021
 Closed

3 tasks
@mschunke
mschunke
commented
Mar 6, 2021

Copy link


Copy Markdown

ContributorAuthor
@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.
@mschunke
mschunke
requested a review
from tux-tnMarch 6, 2021 19:49
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
Sounds good, thank you for the cooperation as well. I missed that yours accept both formatted/unformatted CPFs, that's awesome.
tux-tn
tux-tn
requested changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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
@mschunke
mschunke
commented
Mar 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@tux-tn sorry about that, forget about the readme file! File updated. Please let me know if you need anything else 😁
tux-tn
tux-tn
approved these changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 !
@tux-tntux-tn
removed

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 8, 2021
profnandaa
profnandaa
approved these changes
Mar 12, 2021
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
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! 🎉
@profnandaa
profnandaa
merged commit 3c771e8
into
validatorjs:masterMar 12, 2021
@murilopulley
murilopulley
commented
Mar 12, 2021

Copy link


Copy Markdown

Thanks @profnandaa! Happy to help :)
@profnandaaprofnandaa
mentioned this pull request
Apr 19, 2021
 Closed

@tux-tntux-tn
mentioned this pull request
Oct 4, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@profnandaaprofnandaaprofnandaa approved these changes
@tux-tntux-tntux-tn approved these changes

Assignees


No one assigned


Labels

🎉 first-prneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@mschunke@renanmontebelo@tux-tn@murilopulley@profnandaa

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(pt-BR): tax id, passport and license plates - #1613
Merged
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Mar 12, 2021
Merged
feat(pt-BR): tax id, passport and license plates#1613
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Conversation
@mschunke
@mschunkemschunke
commented
Feb 21, 2021

Copy link


Copy Markdown

Contributor
Included Tax ID, passport, and license plate validators for pt-BR (Brazil) locale. Tests written and passed. Locale added to the documentation.
Checklist
  • [ X ] PR contains only changes related; no stray files, etc.
  • [ X ] README updated (where applicable)
  • [ X ] Tests written (where applicable)
@renanmontebelo
renanmontebelo
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
There's some overlap with these already opened PRs:
We might consider to join forces if you wish.
@mschunke
mschunke
commented
Mar 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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 .isTaxId function and supports also id (CNPJ) validation for entities and well as CPF (persons) validations.
I would suggest to keep his for license plates and mine for tax Ids.
tux-tn
tux-tn
approved these changes
Mar 1, 2021•

edited

Loading
View reviewed changes
tux-tn
tux-tn
requested changes
Mar 1, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 😅
@tux-tntux-tn
added

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 1, 2021
@codecov
codecovBot
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1613 (410ee54) into master (deb1d1e) will not change coverage.

The diff coverage is 100.00%.
Impacted file tree graph
@@ Coverage Diff @@## master #1613 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 100 100 Lines 1807 1849 +42 =========================================
+ Hits 1807 1849 +42 
Impacted FilesCoverage Δ
src/lib/isPassportNumber.js100.00% <ø> (ø)
src/lib/isTaxID.js100.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 deb1d1e...a511310. Read the comment docs.
@mschunke
mschunke
commented
Mar 5, 2021

Copy link


Copy Markdown

ContributorAuthor
Hey @tux-tn, all conflicts are solved and build checks passed. Anything else I should do?
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '88888888888' ||
 tin === '99999999999' ||
 tin === '00000000000'
 ) return false;
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '66666666666666' ||
 tin === '77777777777777' ||
 tin === '88888888888888' ||
 tin === '99999999999999') { return false; }
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
requested changes
Mar 5, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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?
@tux-tn
tux-tn
commented
Mar 5, 2021

Copy link


Copy Markdown

Member
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
Hey, @tux-tn,

Got no reply from @renanmontebelo, but I'm fine with this PR just adding tax id and passport and using the other PR for license plates.
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?
@tux-tn

Was wondering where those untested lines were... thank you! Code updated with 100% coverage.
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
@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 . and -. Please feel free to copy into yours if you think it's a good addition. Thank you.
@renanmontebelorenanmontebelo
mentioned this pull request
Mar 6, 2021
 Closed

3 tasks
@mschunke
mschunke
commented
Mar 6, 2021

Copy link


Copy Markdown

ContributorAuthor
@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.
@mschunke
mschunke
requested a review
from tux-tnMarch 6, 2021 19:49
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
Sounds good, thank you for the cooperation as well. I missed that yours accept both formatted/unformatted CPFs, that's awesome.
tux-tn
tux-tn
requested changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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
@mschunke
mschunke
commented
Mar 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@tux-tn sorry about that, forget about the readme file! File updated. Please let me know if you need anything else 😁
tux-tn
tux-tn
approved these changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 !
@tux-tntux-tn
removed

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 8, 2021
profnandaa
profnandaa
approved these changes
Mar 12, 2021
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
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! 🎉
@profnandaa
profnandaa
merged commit 3c771e8
into
validatorjs:masterMar 12, 2021
@murilopulley
murilopulley
commented
Mar 12, 2021

Copy link


Copy Markdown

Thanks @profnandaa! Happy to help :)
@profnandaaprofnandaa
mentioned this pull request
Apr 19, 2021
 Closed

@tux-tntux-tn
mentioned this pull request
Oct 4, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@profnandaaprofnandaaprofnandaa approved these changes
@tux-tntux-tntux-tn approved these changes

Assignees


No one assigned


Labels

🎉 first-prneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@mschunke@renanmontebelo@tux-tn@murilopulley@profnandaa

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
feat(pt-BR): tax id, passport and license plates - #1613
Merged
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Mar 12, 2021
Merged
feat(pt-BR): tax id, passport and license plates#1613
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Conversation
@mschunke
@mschunkemschunke
commented
Feb 21, 2021

Copy link


Copy Markdown

Contributor
Included Tax ID, passport, and license plate validators for pt-BR (Brazil) locale. Tests written and passed. Locale added to the documentation.
Checklist
  • [ X ] PR contains only changes related; no stray files, etc.
  • [ X ] README updated (where applicable)
  • [ X ] Tests written (where applicable)
@renanmontebelo
renanmontebelo
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
There's some overlap with these already opened PRs:
We might consider to join forces if you wish.
@mschunke
mschunke
commented
Mar 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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 .isTaxId function and supports also id (CNPJ) validation for entities and well as CPF (persons) validations.
I would suggest to keep his for license plates and mine for tax Ids.
tux-tn
tux-tn
approved these changes
Mar 1, 2021•

edited

Loading
View reviewed changes
tux-tn
tux-tn
requested changes
Mar 1, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 😅
@tux-tntux-tn
added

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 1, 2021
@codecov
codecovBot
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1613 (410ee54) into master (deb1d1e) will not change coverage.

The diff coverage is 100.00%.
Impacted file tree graph
@@ Coverage Diff @@## master #1613 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 100 100 Lines 1807 1849 +42 =========================================
+ Hits 1807 1849 +42 
Impacted FilesCoverage Δ
src/lib/isPassportNumber.js100.00% <ø> (ø)
src/lib/isTaxID.js100.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 deb1d1e...a511310. Read the comment docs.
@mschunke
mschunke
commented
Mar 5, 2021

Copy link


Copy Markdown

ContributorAuthor
Hey @tux-tn, all conflicts are solved and build checks passed. Anything else I should do?
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '88888888888' ||
 tin === '99999999999' ||
 tin === '00000000000'
 ) return false;
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '66666666666666' ||
 tin === '77777777777777' ||
 tin === '88888888888888' ||
 tin === '99999999999999') { return false; }
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
requested changes
Mar 5, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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?
@tux-tn
tux-tn
commented
Mar 5, 2021

Copy link


Copy Markdown

Member
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
Hey, @tux-tn,

Got no reply from @renanmontebelo, but I'm fine with this PR just adding tax id and passport and using the other PR for license plates.
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?
@tux-tn

Was wondering where those untested lines were... thank you! Code updated with 100% coverage.
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
@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 . and -. Please feel free to copy into yours if you think it's a good addition. Thank you.
@renanmontebelorenanmontebelo
mentioned this pull request
Mar 6, 2021
 Closed

3 tasks
@mschunke
mschunke
commented
Mar 6, 2021

Copy link


Copy Markdown

ContributorAuthor
@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.
@mschunke
mschunke
requested a review
from tux-tnMarch 6, 2021 19:49
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
Sounds good, thank you for the cooperation as well. I missed that yours accept both formatted/unformatted CPFs, that's awesome.
tux-tn
tux-tn
requested changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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
@mschunke
mschunke
commented
Mar 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@tux-tn sorry about that, forget about the readme file! File updated. Please let me know if you need anything else 😁
tux-tn
tux-tn
approved these changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 !
@tux-tntux-tn
removed

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 8, 2021
profnandaa
profnandaa
approved these changes
Mar 12, 2021
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
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! 🎉
@profnandaa
profnandaa
merged commit 3c771e8
into
validatorjs:masterMar 12, 2021
@murilopulley
murilopulley
commented
Mar 12, 2021

Copy link


Copy Markdown

Thanks @profnandaa! Happy to help :)
@profnandaaprofnandaa
mentioned this pull request
Apr 19, 2021
 Closed

@tux-tntux-tn
mentioned this pull request
Oct 4, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@profnandaaprofnandaaprofnandaa approved these changes
@tux-tntux-tntux-tn approved these changes

Assignees


No one assigned


Labels

🎉 first-prneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@mschunke@renanmontebelo@tux-tn@murilopulley@profnandaa

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(pt-BR): tax id, passport and license plates - #1613
Merged
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Mar 12, 2021
Merged
feat(pt-BR): tax id, passport and license plates#1613
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Conversation
@mschunke
@mschunkemschunke
commented
Feb 21, 2021

Copy link


Copy Markdown

Contributor
Included Tax ID, passport, and license plate validators for pt-BR (Brazil) locale. Tests written and passed. Locale added to the documentation.
Checklist
  • [ X ] PR contains only changes related; no stray files, etc.
  • [ X ] README updated (where applicable)
  • [ X ] Tests written (where applicable)
@renanmontebelo
renanmontebelo
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
There's some overlap with these already opened PRs:
We might consider to join forces if you wish.
@mschunke
mschunke
commented
Mar 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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 .isTaxId function and supports also id (CNPJ) validation for entities and well as CPF (persons) validations.
I would suggest to keep his for license plates and mine for tax Ids.
tux-tn
tux-tn
approved these changes
Mar 1, 2021•

edited

Loading
View reviewed changes
tux-tn
tux-tn
requested changes
Mar 1, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 😅
@tux-tntux-tn
added

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 1, 2021
@codecov
codecovBot
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1613 (410ee54) into master (deb1d1e) will not change coverage.

The diff coverage is 100.00%.
Impacted file tree graph
@@ Coverage Diff @@## master #1613 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 100 100 Lines 1807 1849 +42 =========================================
+ Hits 1807 1849 +42 
Impacted FilesCoverage Δ
src/lib/isPassportNumber.js100.00% <ø> (ø)
src/lib/isTaxID.js100.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 deb1d1e...a511310. Read the comment docs.
@mschunke
mschunke
commented
Mar 5, 2021

Copy link


Copy Markdown

ContributorAuthor
Hey @tux-tn, all conflicts are solved and build checks passed. Anything else I should do?
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '88888888888' ||
 tin === '99999999999' ||
 tin === '00000000000'
 ) return false;
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '66666666666666' ||
 tin === '77777777777777' ||
 tin === '88888888888888' ||
 tin === '99999999999999') { return false; }
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
requested changes
Mar 5, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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?
@tux-tn
tux-tn
commented
Mar 5, 2021

Copy link


Copy Markdown

Member
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
Hey, @tux-tn,

Got no reply from @renanmontebelo, but I'm fine with this PR just adding tax id and passport and using the other PR for license plates.
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?
@tux-tn

Was wondering where those untested lines were... thank you! Code updated with 100% coverage.
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
@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 . and -. Please feel free to copy into yours if you think it's a good addition. Thank you.
@renanmontebelorenanmontebelo
mentioned this pull request
Mar 6, 2021
 Closed

3 tasks
@mschunke
mschunke
commented
Mar 6, 2021

Copy link


Copy Markdown

ContributorAuthor
@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.
@mschunke
mschunke
requested a review
from tux-tnMarch 6, 2021 19:49
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
Sounds good, thank you for the cooperation as well. I missed that yours accept both formatted/unformatted CPFs, that's awesome.
tux-tn
tux-tn
requested changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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
@mschunke
mschunke
commented
Mar 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@tux-tn sorry about that, forget about the readme file! File updated. Please let me know if you need anything else 😁
tux-tn
tux-tn
approved these changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 !
@tux-tntux-tn
removed

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 8, 2021
profnandaa
profnandaa
approved these changes
Mar 12, 2021
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
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! 🎉
@profnandaa
profnandaa
merged commit 3c771e8
into
validatorjs:masterMar 12, 2021
@murilopulley
murilopulley
commented
Mar 12, 2021

Copy link


Copy Markdown

Thanks @profnandaa! Happy to help :)
@profnandaaprofnandaa
mentioned this pull request
Apr 19, 2021
 Closed

@tux-tntux-tn
mentioned this pull request
Oct 4, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@profnandaaprofnandaaprofnandaa approved these changes
@tux-tntux-tntux-tn approved these changes

Assignees


No one assigned


Labels

🎉 first-prneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@mschunke@renanmontebelo@tux-tn@murilopulley@profnandaa

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(pt-BR): tax id, passport and license plates - #1613
Merged
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Mar 12, 2021
Merged
feat(pt-BR): tax id, passport and license plates#1613
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Conversation
@mschunke
@mschunkemschunke
commented
Feb 21, 2021

Copy link


Copy Markdown

Contributor
Included Tax ID, passport, and license plate validators for pt-BR (Brazil) locale. Tests written and passed. Locale added to the documentation.
Checklist
  • [ X ] PR contains only changes related; no stray files, etc.
  • [ X ] README updated (where applicable)
  • [ X ] Tests written (where applicable)
@renanmontebelo
renanmontebelo
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
There's some overlap with these already opened PRs:
We might consider to join forces if you wish.
@mschunke
mschunke
commented
Mar 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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 .isTaxId function and supports also id (CNPJ) validation for entities and well as CPF (persons) validations.
I would suggest to keep his for license plates and mine for tax Ids.
tux-tn
tux-tn
approved these changes
Mar 1, 2021•

edited

Loading
View reviewed changes
tux-tn
tux-tn
requested changes
Mar 1, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 😅
@tux-tntux-tn
added

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 1, 2021
@codecov
codecovBot
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1613 (410ee54) into master (deb1d1e) will not change coverage.

The diff coverage is 100.00%.
Impacted file tree graph
@@ Coverage Diff @@## master #1613 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 100 100 Lines 1807 1849 +42 =========================================
+ Hits 1807 1849 +42 
Impacted FilesCoverage Δ
src/lib/isPassportNumber.js100.00% <ø> (ø)
src/lib/isTaxID.js100.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 deb1d1e...a511310. Read the comment docs.
@mschunke
mschunke
commented
Mar 5, 2021

Copy link


Copy Markdown

ContributorAuthor
Hey @tux-tn, all conflicts are solved and build checks passed. Anything else I should do?
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '88888888888' ||
 tin === '99999999999' ||
 tin === '00000000000'
 ) return false;
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '66666666666666' ||
 tin === '77777777777777' ||
 tin === '88888888888888' ||
 tin === '99999999999999') { return false; }
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
requested changes
Mar 5, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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?
@tux-tn
tux-tn
commented
Mar 5, 2021

Copy link


Copy Markdown

Member
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
Hey, @tux-tn,

Got no reply from @renanmontebelo, but I'm fine with this PR just adding tax id and passport and using the other PR for license plates.
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?
@tux-tn

Was wondering where those untested lines were... thank you! Code updated with 100% coverage.
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
@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 . and -. Please feel free to copy into yours if you think it's a good addition. Thank you.
@renanmontebelorenanmontebelo
mentioned this pull request
Mar 6, 2021
 Closed

3 tasks
@mschunke
mschunke
commented
Mar 6, 2021

Copy link


Copy Markdown

ContributorAuthor
@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.
@mschunke
mschunke
requested a review
from tux-tnMarch 6, 2021 19:49
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
Sounds good, thank you for the cooperation as well. I missed that yours accept both formatted/unformatted CPFs, that's awesome.
tux-tn
tux-tn
requested changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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
@mschunke
mschunke
commented
Mar 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@tux-tn sorry about that, forget about the readme file! File updated. Please let me know if you need anything else 😁
tux-tn
tux-tn
approved these changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 !
@tux-tntux-tn
removed

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 8, 2021
profnandaa
profnandaa
approved these changes
Mar 12, 2021
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
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! 🎉
@profnandaa
profnandaa
merged commit 3c771e8
into
validatorjs:masterMar 12, 2021
@murilopulley
murilopulley
commented
Mar 12, 2021

Copy link


Copy Markdown

Thanks @profnandaa! Happy to help :)
@profnandaaprofnandaa
mentioned this pull request
Apr 19, 2021
 Closed

@tux-tntux-tn
mentioned this pull request
Oct 4, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@profnandaaprofnandaaprofnandaa approved these changes
@tux-tntux-tntux-tn approved these changes

Assignees


No one assigned


Labels

🎉 first-prneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@mschunke@renanmontebelo@tux-tn@murilopulley@profnandaa

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
feat(pt-BR): tax id, passport and license plates - #1613
Merged
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Mar 12, 2021
Merged
feat(pt-BR): tax id, passport and license plates#1613
profnandaa merged 6 commits into
validatorjs:masterfrom 
mschunke:master
Conversation
@mschunke
@mschunkemschunke
commented
Feb 21, 2021

Copy link


Copy Markdown

Contributor
Included Tax ID, passport, and license plate validators for pt-BR (Brazil) locale. Tests written and passed. Locale added to the documentation.
Checklist
  • [ X ] PR contains only changes related; no stray files, etc.
  • [ X ] README updated (where applicable)
  • [ X ] Tests written (where applicable)
@renanmontebelo
renanmontebelo
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
There's some overlap with these already opened PRs:
We might consider to join forces if you wish.
@mschunke
mschunke
commented
Mar 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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 .isTaxId function and supports also id (CNPJ) validation for entities and well as CPF (persons) validations.
I would suggest to keep his for license plates and mine for tax Ids.
tux-tn
tux-tn
approved these changes
Mar 1, 2021•

edited

Loading
View reviewed changes
tux-tn
tux-tn
requested changes
Mar 1, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 😅
@tux-tntux-tn
added

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 1, 2021
@codecov
codecovBot
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1613 (410ee54) into master (deb1d1e) will not change coverage.

The diff coverage is 100.00%.
Impacted file tree graph
@@ Coverage Diff @@## master #1613 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 100 100 Lines 1807 1849 +42 =========================================
+ Hits 1807 1849 +42 
Impacted FilesCoverage Δ
src/lib/isPassportNumber.js100.00% <ø> (ø)
src/lib/isTaxID.js100.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 deb1d1e...a511310. Read the comment docs.
@mschunke
mschunke
commented
Mar 5, 2021

Copy link


Copy Markdown

ContributorAuthor
Hey @tux-tn, all conflicts are solved and build checks passed. Anything else I should do?
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '88888888888' ||
 tin === '99999999999' ||
 tin === '00000000000'
 ) return false;
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
reviewed
Mar 5, 2021
View reviewed changes
Comment threadsrc/lib/isTaxID.js
 tin === '66666666666666' ||
 tin === '77777777777777' ||
 tin === '88888888888888' ||
 tin === '99999999999999') { return false; }
@tux-tntux-tnMar 5, 2021

Copy link


Copy Markdown

Member
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
tux-tn
tux-tn
requested changes
Mar 5, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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?
@tux-tn
tux-tn
commented
Mar 5, 2021

Copy link


Copy Markdown

Member
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@renanmontebelo@mschunke what have you decided concerning what approach to keep?

Should this one add only tax id and passport and the other one add license plate validation?
Hey, @tux-tn,

Got no reply from @renanmontebelo, but I'm fine with this PR just adding tax id and passport and using the other PR for license plates.
@mschunke
mschunke
commented
Mar 5, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?
@tux-tn

Was wondering where those untested lines were... thank you! Code updated with 100% coverage.
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
@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 . and -. Please feel free to copy into yours if you think it's a good addition. Thank you.
@renanmontebelorenanmontebelo
mentioned this pull request
Mar 6, 2021
 Closed

3 tasks
@mschunke
mschunke
commented
Mar 6, 2021

Copy link


Copy Markdown

ContributorAuthor
@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.
@mschunke
mschunke
requested a review
from tux-tnMarch 6, 2021 19:49
@renanmontebelo
renanmontebelo
commented
Mar 6, 2021

Copy link


Copy Markdown

Contributor
Sounds good, thank you for the cooperation as well. I missed that yours accept both formatted/unformatted CPFs, that's awesome.
tux-tn
tux-tn
requested changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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
@mschunke
mschunke
commented
Mar 8, 2021

Copy link


Copy Markdown

ContributorAuthor
@tux-tn sorry about that, forget about the readme file! File updated. Please let me know if you need anything else 😁
tux-tn
tux-tn
approved these changes
Mar 8, 2021
View reviewed changes
@tux-tntux-tn
left a comment


Copy link


Copy Markdown

Member
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 !
@tux-tntux-tn
removed

🍿 discussion

🧹 needs-update
For PRs that need to be updated before landing
labels
Mar 8, 2021
profnandaa
profnandaa
approved these changes
Mar 12, 2021
View reviewed changes
@profnandaaprofnandaa
left a comment


Copy link


Copy Markdown

Member
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! 🎉
@profnandaa
profnandaa
merged commit 3c771e8
into
validatorjs:masterMar 12, 2021
@murilopulley
murilopulley
commented
Mar 12, 2021

Copy link


Copy Markdown

Thanks @profnandaa! Happy to help :)
@profnandaaprofnandaa
mentioned this pull request
Apr 19, 2021
 Closed

@tux-tntux-tn
mentioned this pull request
Oct 4, 2021
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@profnandaaprofnandaaprofnandaa approved these changes
@tux-tntux-tntux-tn approved these changes

Assignees


No one assigned


Labels

🎉 first-prneeds-more-review

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@mschunke@renanmontebelo@tux-tn@murilopulley@profnandaa