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.

Sign up for GitHub

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

Jump to bottom

[en_GB] VAT number faker #255

Merged
merged 7 commits into from
Jan 8, 2021
Merged

[en_GB] VAT number faker #255

merged 7 commits into from
Jan 8, 2021

Conversation

dmlogic
Copy link

@dmlogic dmlogic commented Jan 7, 2021 •
edited
Loading

What is the reason for this PR?

  • A new feature

Author's checklist

Summary of changes

  • Added a en_GB\Company provider to fake VAT numbers
  • Documentation PR is here

Review checklist

  • All checks have passed
  • Changes are approved by maintainer

@dmlogic dmlogic mentioned this pull request Jan 7, 2021
Merged
pimjansen
pimjansen requested changes Jan 7, 2021
View reviewed changes
src/Faker/Provider/en_GB/Company.php Outdated Show resolved Hide resolved
src/Faker/Provider/en_GB/Company.php Outdated Show resolved Hide resolved
test/Faker/Provider/en_GB/CompanyTest.php Outdated Show resolved Hide resolved
krsriq
krsriq reviewed Jan 7, 2021
View reviewed changes
src/Faker/Provider/en_GB/Company.php Outdated
return sprintf(
'%s%d %d %d %d',
static::VAT_PREFIX,
static::randomNumber(3, true),
Copy link

Choose a reason for hiding this comment

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

This only produces numbers bigger than 100. Is that on purpose? If not you could use the str_pad solution from below here too. The other cases that use randomNumber have the same issue.

Copy link
Author

Choose a reason for hiding this comment

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

Personally I'd rather stick with the simpler randomNumber() generator where possible. If I was wanting to test the behaviour of numbers with leading zeros I would use a defined value rather than faking it.

Copy link

@krsriq krsriq Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You could also use static::numerify('### #### ## ###').

src/Faker/Provider/en_GB/Company.php Outdated Show resolved Hide resolved
@dmlogic
Copy link
Author

dmlogic commented Jan 8, 2021

I re-read the Wikipedia notes about this and have subsequently re-worked things a bit. Numbers are now much more likely to pass validation checks and hopefully some of the other issues raised have gone away.

krsriq
krsriq suggested changes Jan 8, 2021
View reviewed changes
test/Faker/Provider/en_GB/CompanyTest.php Outdated Show resolved Hide resolved
@bram-pkg bram-pkg changed the title UK VAT number faker [en_GB] VAT number faker Jan 8, 2021
@pimjansen pimjansen added the enhancement New feature or request label Jan 8, 2021
@bram-pkg bram-pkg requested a review from pimjansen January 8, 2021 15:45
@pimjansen pimjansen merged commit 7e6c6b0 into FakerPHP:main Jan 8, 2021
@dmlogic
Copy link
Author

dmlogic commented Jan 8, 2021

Thank you

@lukasjuhas lukasjuhas mentioned this pull request Mar 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krsriq krsriq krsriq requested changes

@bram-pkg bram-pkg bram-pkg approved these changes

@pimjansen pimjansen pimjansen approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmlogic @pimjansen @krsriq @bram-pkg