-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: introduce distribute()
method
#826
feat: introduce distribute()
method
#826
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks cool. Admittedly, I find it hard to reason about. I'd love to see some concrete examples in the docs to help visualize the end result (one with relations).
src/Factory.php
Outdated
* | ||
* @return list<T> | ||
*/ | ||
final public static function createForEach(string $field, array $values): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this is common enough to warrant a dedicated static method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope you're right, I'll remove it
nice :)
yeah, this might be for some edge cases only
I'll add concrete examples in the docs. I have plenty of cases where this could be useful in the project I'm working on I'm still looking for better names for the first one 🤔 |
Oo I do like |
let's go for
not sure what you mean:
|
My bad, I misread the signature for mapEach. Does it make sense for this one to include "spread" in the name? |
Not sure... we actually "spread" the values to a callable which will be used in a I'm wondering if |
Got it.
Feels like a bit of a mouthful. Let's keep this one as is |
I love the feature (and we needed exactly this on the work project :)) But yes we can do better for the API :)
The You suggest to use spread then
... and i'm not sure it really is perfect either... Iwould suggest .... (and taken from the PR comment)
|
hi @smnandre thanks for this proposal, |
95a5d18
to
4a5e003
Compare
distribute()
method
4a5e003
to
439d9ee
Compare
6a5fcde
to
4da79cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call to update this PR to just distribute()
.
{ | ||
$factories = $this->all(); | ||
|
||
if (\count($factories) !== \count($values)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perfect for a start.
I wonder if we could allow some options there in the future
A couple of ideas:
Cycle
categories : A, B
posts: P1, P2, P3
P1: A
P2: B
P3: A
All
categories : A, B, C
posts: P1, P2
P1: A, C
P2: B
Ignore missing
categories : A, B
posts: P1, P2, P3
P1: A
P2: B
P3:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we can totally pass a third parameter $strategy
or something like that! Let's see what kind of use case will be needed
4da79cc
to
b3f8b40
Compare
I wanted since a lot of time improve a little bit the API around collections, to facilitate complex cases with one-to-many, mainly to be used with sequences.
here is one new method:
FactoryCollection::distribute($field, $values)
: it can be use to distribute the values to all factories inside the collection for a given field: