Skip to content

Conversation

@wiktorwojcik112
Copy link
Contributor

Add isPESEL validation.

PESEL is a polish national identification number. Validator checks whether provided string follows criteria of
valid PESEL.

Checklist

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

@codecov
Copy link

codecov bot commented Oct 2, 2021

Codecov Report

Merging #1745 (ec4801d) into master (c899b31) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1745   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          102       102           
  Lines         2015      2027   +12     
  Branches       454       456    +2     
=========================================
+ Hits          2015      2027   +12     
Impacted Files Coverage Δ
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 c899b31...ec4801d. Read the comment docs.

Copy link
Contributor

@fedeci fedeci left a comment

Choose a reason for hiding this comment

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

Does it have anything to do with isIdentityCard? I am quite in disagreement with adding a specific validator for a specific country code.

@wiktorwojcik112
Copy link
Contributor Author

Does it have anything to do with isIdentityCard? I am quite in disagreement with adding a specific validator for a specific country code.

I checked and it would fit there. I will add this PESEL validator to isIdentityCard.

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 @wiktorwojcik112
Totally agree with @fedeci this validator should be added as a locale to isIdentityCard. Please make the necessary changes and add tests for the uncovered line in your code

@tux-tn tux-tn added 🎉 first-pr 🧹 needs-update For PRs that need to be updated before landing needs-tests labels Oct 2, 2021
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 updating your PR @wiktorwojcik112 can you please fix the linter issue and add the missing test cases to bring back coverage to 100%.
I suggest you run tests locally before pushing new code (using yarn test or npm run test)
EDIT: Can you please change the PR title to specify you are adding a new locale to isIdentityCard and not a new validator (The PR title will be the commit message when the PR is merged and the actual title is misleading)

@wiktorwojcik112 wiktorwojcik112 changed the title feat(isPESEL): Add validator for PESEL feat(isIdentityCard): Add PL locale Oct 2, 2021
@wiktorwojcik112
Copy link
Contributor Author

Ok. I fixed linter error and changed title, but I still can't figure out how to solve coverage issue.

@tux-tn
Copy link
Member

tux-tn commented Oct 2, 2021

Ok. I fixed linter error and changed title, but I still can't figure out how to solve coverage issue.

That's tricky but you need to find a test example where the last digit of PESEL number equals 10 minus your modulo.
As i told you earlier the uncovered execution path is lastDigit === 10 - modulo

@wiktorwojcik112
Copy link
Contributor Author

Ok. I fixed linter error and changed title, but I still can't figure out how to solve coverage issue.

That's tricky but you need to find a test example where the last digit of PESEL number equals 10 minus your modulo. As i told you earlier the uncovered execution path is lastDigit === 10 - modulo

Everything should be good now.

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 @wiktorwojcik112 can you please address my latest comment concerning istanbul ignore else ?

@wiktorwojcik112 wiktorwojcik112 requested a review from tux-tn October 2, 2021 15:11
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 for taking the time to quickly address all the comments

@tux-tn tux-tn added ready-to-land For PRs that are reviewed and ready to be landed and removed 🧹 needs-update For PRs that need to be updated before landing needs-tests labels Oct 2, 2021
@wiktorwojcik112
Copy link
Contributor Author

wiktorwojcik112 commented Oct 2, 2021

LGTM 🎉 Thank you for taking the time to quickly address all the comments

It was nice working with you. Thank you all for help. Take care!

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

@profnandaa profnandaa merged commit f2a1587 into validatorjs:master Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🎉 first-pr 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