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

allows passing an enum to randomElement() or randomElements() #620

Merged
merged 15 commits into from
May 14, 2023
Merged

allows passing an enum to randomElement() or randomElements() #620

merged 15 commits into from
May 14, 2023

Conversation

cosmastech
Copy link

@cosmastech cosmastech commented Mar 29, 2023 •
edited by localheinz
Loading

What is the reason for this PR?

faker()->randomElement(SomeEnum::cases())

is fine, but I think it would be even nicer to have

faker()->randomElement(SomeEnum::class);
  • A new feature

Author's checklist

Summary of changes

Review checklist

  • All checks have passed
  • Changes are approved by maintainer

@localheinz localheinz changed the title allows passing an enum to randomElement or randomElements allows passing an enum to randomElement() or randomElements() Apr 1, 2023
@localheinz localheinz changed the title allows passing an enum to randomElement() or randomElements() allows passing an enum to randomElement() or randomElements() Apr 1, 2023
@cosmastech
Copy link
Author

@localheinz is there I need to do with the failing pipeline items? The enum_exists issue is already accounted for by checking if the function exists.

Didn't look into the \Traversable thing too closely.

@stale
Copy link

stale bot commented Apr 25, 2023

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.

localheinz
localheinz reviewed Apr 27, 2023
View reviewed changes
src/Faker/Provider/Base.php Outdated Show resolved Hide resolved
src/Faker/Provider/Base.php Outdated Show resolved Hide resolved
@localheinz localheinz self-assigned this Apr 27, 2023
@localheinz localheinz added the enhancement New feature or request label Apr 27, 2023
localheinz
localheinz approved these changes Apr 27, 2023
View reviewed changes
Copy link
Member

@localheinz localheinz left a comment

Choose a reason for hiding this comment

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

I think we can simplify this a bit, but it looks good to me otherwise!

@cosmastech
Copy link
Author

I think we can simplify this a bit, but it looks good to me otherwise!

Thanks @localheinz!

@localheinz localheinz force-pushed the feature/allow-random-element-from-enum branch from eea2bd5 to f58c881 Compare April 27, 2023 13:48
cosmastech
cosmastech commented Apr 27, 2023
View reviewed changes
src/Faker/Provider/Base.php Outdated
@@ -192,6 +192,10 @@ public static function randomElements($array = ['a', 'b', 'c'], $count = 1, $all
$traversables = [];
$arr = $array;

if (is_string($array) && function_exists('enum_exists') && enum_exists($array)) {
$traversables = $array::cases();
Copy link
Author

Choose a reason for hiding this comment

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

If we change this to $arr = $traversables = $array::cases() the test passes, but I'm still unsure why it was failing in the first place

@localheinz localheinz mentioned this pull request Apr 27, 2023
Merged
5 tasks
@localheinz
Copy link
Member

@cosmastech

I find the code hard to understand in the first place and I am trying to clean it up in #639!

@localheinz localheinz force-pushed the feature/allow-random-element-from-enum branch from 2f21b41 to 3ac0b61 Compare May 1, 2023 10:36
@localheinz localheinz mentioned this pull request May 1, 2023
Merged
1 task
@localheinz localheinz assigned pimjansen and bram-pkg and unassigned localheinz May 1, 2023
cosmastech and others added 11 commits May 2, 2023 10:07
@cosmastech @localheinz
specify class-string
5246503
@cosmastech @localheinz
Update test/Faker/Provider/BaseTest.php
674e29c 
Co-authored-by: Andreas Möller <am@localheinz.com>
@cosmastech @localheinz
remove require_once
e0fcd03 
Co-authored-by: Andreas Möller <am@localheinz.com>
@cosmastech @localheinz
clean up tests
a61341d 
splits tests, splits phases of tests, adds more specific assertions

Co-authored-by: Andreas Möller <am@localheinz.com>
@cosmastech @localheinz
Apply suggestions from code review
b15f47c 
Co-authored-by: Andreas Möller <am@localheinz.com>
@localheinz
Fix: Avoid unnecessary import
1116450
@localheinz
Fix: Remove unnecessary reference to root namespace
6730fe4
@localheinz
Fix: Exclude BackedEnum
21544ff
@localheinz
Fix: DocBlock
9d9e917
@localheinz
Fix: Run 'make baseline'
6b378a9
@localheinz
Fix: Run 'make baseline'
021e8d3
@localheinz localheinz force-pushed the feature/allow-random-element-from-enum branch from 5460815 to 021e8d3 Compare May 2, 2023 08:09
@localheinz localheinz merged commit 096f7ff into FakerPHP:main May 14, 2023
@localheinz
Copy link
Member

Thank you, @cosmastech!

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

@localheinz localheinz localheinz approved these changes

@pimjansen pimjansen Awaiting requested review from pimjansen

@bram-pkg bram-pkg Awaiting requested review from bram-pkg

Assignees

@pimjansen pimjansen

@bram-pkg bram-pkg

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

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