Skip to content

Conversation

@mhf-ir
Copy link
Contributor

@mhf-ir mhf-ir commented Feb 7, 2021

  • IR for isPassportNumber
  • IR for isIdentityCard
  • respect .gitignore

Checklist

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

@codecov
Copy link

codecov bot commented Feb 7, 2021

Codecov Report

Merging #1595 (7e2aa16) into master (63b6162) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1595   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         1843      1852    +9     
=========================================
+ Hits          1843      1852    +9     
Impacted Files Coverage Δ
src/lib/isPassportNumber.js 100.00% <ø> (ø)
src/lib/isIdentityCard.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 63b6162...7e2aa16. Read the comment docs.

Co-authored-by: Federico Ciardi <[email protected]>
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 PR @mhf-ir and sorry for the long review delay!
Your additions to isIdentityCard and isPassportNumber are good to go;
I'm not sure about adding isLuhn as a standalone validator, i don't see a big usage outside of credit card number validation (isCreditCard is already using it internally) and we should probably open this for discussion and wait for @ezkemboi , @profnandaa and the rest of the community feedback.

@mhf-ir
Copy link
Contributor Author

mhf-ir commented Mar 17, 2021

@tux-tn As i Said previously isLuhn one of the most important validation that you can see in npm many version (about 50 package) of that is implemented.

  • Fact 1: Validator.js need isLuhn

Forget about isCreditCard, it's Independent validation for Luhn algorithm that developers need it already. Dependency of isCreditCard is out of this PR scope.

isCreditCard might be depend on Luhn or not, but could be different issue and PR.

  • Problem 1:
    isCreditCard might using isLuhn or not. So fix it in different issue/PR.

@mhf-ir
Copy link
Contributor Author

mhf-ir commented Mar 17, 2021

After all I can remove isLuhn and send another issue and PR. Is that OK? @tux-tn

@mhf-ir mhf-ir requested a review from tux-tn March 17, 2021 10:09
@tux-tn
Copy link
Member

tux-tn commented Mar 17, 2021

After all I can remove isLuhn and send another issue and PR. Is that OK? @tux-tn

it's a good compromise, it will allow us to land directly IR Passport/IdentityCard validator and open discussion concerning the new isLuhn validator (which may take some time)

@mhf-ir mhf-ir changed the title feat: isLuhn, IR passport and identityCard, respect .gitignore files feat: IR passport and identityCard, respect .gitignore files Mar 17, 2021
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.

LGTM 🎉
Thank you again @mhf-ir for taking the time to fix conflicts and make the necessary changes!

@tux-tn tux-tn added ready-to-land For PRs that are reviewed and ready to be landed and removed needs-more-review labels Mar 17, 2021
@tux-tn
Copy link
Member

tux-tn commented Mar 17, 2021

@mhf-ir feel free to open a new PR concerning isLuhn in order to continue the discussion

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.

LGTM, thanks for your contrib! 🎉

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

Labels

ready-to-land For PRs that are reviewed and ready to be landed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants