Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't generate birth number of '000' for Swedish personal identity number #306

Merged
merged 8 commits into from
May 24, 2021

Conversation

pelmered
Copy link

@pelmered pelmered commented Apr 7, 2021

What is the reason for this PR?

Author's checklist

Summary of changes

Swedish personal identity number with a birth number of 000 is not valid. The birth number is the first 3 numbers of the last 4, marked with X here 19900101-XXX0

Note: There are no tests for this. I'm not sure how to test this in a good way as this only happen one time in 1000 on average. Any suggestions for this, or can we skip the test?

Review checklist

  • All checks have passed
  • Changes are approved by maintainer

@pimjansen
Copy link

Missing tests for this

@bram-pkg
Copy link
Member

@pelmered this looks fine once you add the early return mentioned in @powellblyth's reply.

@bram-pkg bram-pkg added bug Something isn't working needs work labels Apr 15, 2021
@bram-pkg bram-pkg self-assigned this Apr 15, 2021
@stale
Copy link

stale bot commented Apr 30, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 1 week if no further activity occurs. Thank you for your contributions.

@pelmered
Copy link
Author

pelmered commented May 13, 2021

@bramceulemans
Updated the code now with early returns as requested.
However, I think the code is considerable less readable now than it was before with the else statement.

@pimjansen pimjansen requested a review from bram-pkg May 13, 2021 12:22
@pimjansen
Copy link

@bramceulemans

Updated the code now with early returns as requested.

However, I think the code is considerable less readable now than it was before with the else statement.

Still a small change requested. For the rest it looks fine for me

Copy link
Member

@bram-pkg bram-pkg left a comment

Choose a reason for hiding this comment

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

If statements should be type-safe.

@pelmered pelmered requested a review from bram-pkg May 24, 2021 09:40
Copy link
Member

@bram-pkg bram-pkg 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 the effort of improving this PR.

@bram-pkg
Copy link
Member

@pimjansen what do you think about the suggestions from php-cs-fixer?

@pimjansen
Copy link

@pimjansen what do you think about the suggestions from php-cs-fixer?

Well not really an opinion though. There are pro and cons for both. But we have the fixer so lets follow it.

@bram-pkg
Copy link
Member

Thanks for the quick fix.

@bram-pkg bram-pkg merged commit abde3b0 into FakerPHP:main May 24, 2021
@pelmered
Copy link
Author

Thanks for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Birth numbers of '000' is not valid for Swedish personal identity number
5 participants