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

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

Merged
merged 1 commit into from
Jun 10, 2023

Conversation

calebdw
Copy link

@calebdw calebdw commented Jun 9, 2023

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

@bram-pkg
Copy link
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
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
Member

bram-pkg commented Jun 9, 2023

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
Author

calebdw commented Jun 9, 2023

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

@bram-pkg
Copy link
Member

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

DvDty commented Jun 10, 2023

@bram-pkg can the community help with this?

@calebdw
Copy link
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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants