Skip to content

Update randomElements to return random number of elements when no count is provided#658

Merged
bram-pkg merged 1 commit intoFakerPHP:mainfrom
calebdw:randomElements
Jun 10, 2023
Merged

Update randomElements to return random number of elements when no count is provided#658
bram-pkg merged 1 commit intoFakerPHP:mainfrom
calebdw:randomElements

Conversation

@calebdw
Copy link
Copy Markdown

@calebdw calebdw commented Jun 9, 2023 •
edited
Loading

Hello!

What is the reason for this PR?

This updates randomElements to return a random number of elements when no count is provided. Defaulting to a count of 1 is undesirable because if I only wanted one element I would just use randomElement. This cleans up having to pass fake()->numberBetween(1, count($array)) as the $count when I don't care how many values are returned.

  • A new feature
  • Fixed an issue (resolve #ID)

Author's checklist

Summary of changes

  • Made $count in Base::randomElement nullable, and if null, then default to random number of elements.
  • Updated documentation

Review checklist

  • All checks have passed
  • Changes are approved by maintainer

@calebdw calebdw mentioned this pull request Jun 9, 2023
Merged
@calebdw calebdw force-pushed the randomElements branch 3 times, most recently from 07e73e4 to de7cc0b Compare June 9, 2023 14:45
@bram-pkg
Copy link
Copy Markdown
Member

bram-pkg commented Jun 9, 2023

Unfortunately changing the default value of a parameter is a breaking change if it changes the output of the function.

@calebdw
Copy link
Copy Markdown
Author

calebdw commented Jun 9, 2023

I understand, can we merge to the next major version though? I don't think it really makes sense to have to explicitly pass null to $count for this to behave as it reads.

Or I suppose I could change the default back to 1, and then on the next major release we could update the default value

@bram-pkg
Copy link
Copy Markdown
Member

bram-pkg commented Jun 9, 2023 •
edited
Loading

Indeed, I think allowing null to get a random number of elements sounds fine, but the signature cannot change within 1.x, sorry.

Please be sure to include a test that asserts a count, which should work fine since we are seeding the mt_ functions.

Also please open a PR to the docs repository.

@calebdw calebdw force-pushed the randomElements branch 2 times, most recently from 5c4c973 to a4a62f9 Compare June 9, 2023 15:52
@calebdw
Copy link
Copy Markdown
Author

calebdw commented Jun 9, 2023

@bram-pkg done, is there a planned 2.x release date?

@bram-pkg
Copy link
Copy Markdown
Member

bram-pkg commented Jun 10, 2023

Not as of yet. We're struggling to be motivated I think, outside our day jobs :)

@bram-pkg bram-pkg merged commit f64484a into FakerPHP:main Jun 10, 2023
@calebdw calebdw deleted the randomElements branch June 10, 2023 11:46
@DvDty
Copy link
Copy Markdown

DvDty commented Jun 10, 2023

@bram-pkg can the community help with this?

@calebdw
Copy link
Copy Markdown
Author

calebdw commented Jun 10, 2023

That's a good idea, it would be helpful if a 'Roadmap to 2.0' issue was created with what needs to be done and then the community could pitch in

@pimjansen
Copy link
Copy Markdown

pimjansen commented Jun 11, 2023

That's a good idea, it would be helpful if a 'Roadmap to 2.0' issue was created with what needs to be done and then the community could pitch in

Will work on that

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

Reviewers

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@calebdw @bram-pkg @DvDty @pimjansen