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

[8.x] Track a loop variable for sequence and pass it with count to closure #37799

Merged
merged 4 commits into from
Jun 25, 2021

Conversation

jimmypuckett
Copy link
Contributor

@jimmypuckett jimmypuckett commented Jun 24, 2021

I have a use case where I would like to know the loop count in a sequence, so I thought that others might like the ability too.

All this does is track a loop variable similar to foreach to use with % to get the index. Then it passes it to the closure.

Model::factory()->count(5)
    ->sequence(fn($loop, $count) => ['sort' => $loop * 100])
    ->create();

$loop & $count is optional, and people would only declare if needed. I almost did not pass $count, but there are times that we do a random number in the count method, so there is a slight use case.

This is a re-mix of #37753 in a way to be backward compatible, as @driesvints suggested.

Thanks for considering & I hope that you see value in it.

Thanks so much for all the work that you put into Laravel!

@taylorotwell
Copy link
Member

So - what is "loop"? I'm not sure you actually explain what you mean by "loop count"?

@jimmypuckett
Copy link
Contributor Author

jimmypuckett commented Jun 24, 2021

@taylorotwell Sorry that the use case is not more clear.

For my specific case, I have a model Recommendation that belongsTo Section. The user can drag & drop the recommendations into a specific sort order, so there is a sort property on Recommendation.

I needed a way to increment the sort for each loop, but currently, there is not a way to know the loop count in the sequence, so we are doing so "extra" code.

Here is a very simple example...

$sorts = Collection::times(rand(1, 5), fn ($i) => ['sort' => ($i + 1) * 100])->toArray();

$count = count($sorts);

return $this->has(
    Section::factory()->count($count)
        ->sequence(...$sorts),
    'recommendations'
)->state(fn () => ['sort' => $sort]);

If sequence kept up with the loop count, then we could do...

 return $this->has(
    Section::factory()->count(rand(1, 5))
        ->sequence(fn($loop) => ['sort' => ($loop +1)*100]),
    'recommendations'
);

I hope this is more clear. In the end, just looking to optionally know in the sequence closure the number of times that the sequence has looped & how many times it is looping.

Thanks again for considering this.

@taylorotwell
Copy link
Member

taylorotwell commented Jun 24, 2021

Why not just make the $count and $index properties public on the Sequence class and then pass $this into the callback? That way you can access all information on the sequence instance itself directly (and any additional stuff that ever exists here).

@jimmypuckett
Copy link
Contributor Author

jimmypuckett commented Jun 24, 2021

I really like the idea of passing $this, but unless that I am missing something, if you only have $index & $count as they currently exist, then there is no way to know the number of times the sequence has run.

Personally, I would like to repurpose $index as that loop count & calculate the sequence index when needed.
If you think that this adds value, we could do...

<?php

namespace Illuminate\Database\Eloquent\Factories;

class Sequence
{
    /**
     * The sequence of return values.
     *
     * @var array
     */
    protected $sequence;

    /**
     * The count of the sequence items.
     *
     * @var int
     */
    public $count;

    /**
     * The current index of the sequence iteration.
     *
     * @var int
     */
    public $index = 0;

    /**
     * Create a new sequence instance.
     *
     * @param  array  $sequence
     * @return void
     */
    public function __construct(...$sequence)
    {
        $this->sequence = $sequence;
        $this->count = count($sequence);
    }

    /**
     * Get the next value in the sequence.
     *
     * @return mixed
     */
    public function __invoke()
    {
        return tap(value($this->sequence[$this->index % $this->count], $this), function () {
            $this->index = $this->index + 1;
        });
    }
}

Then the example above would be...

return $this->has(
    Section::factory()->count(rand(1, 5))
        ->sequence(fn(Sequence $sequence) => ['sort' => ($sequence->index + 1) * 100]),
    'recommendations'
);

I would be happy to do whichever you prefer.

@taylorotwell
Copy link
Member

Since $index is protected I think it is safe to implement that suggestion. If you want to modify this PR to reflect that go ahead.

@jimmypuckett
Copy link
Contributor Author

Since we are going to expose a public variable, should we consider making the "API" emulate the $loop variable in blade with similar properties or would that be overkill?

@taylorotwell
Copy link
Member

Overkill IMO for now. Nobody asking for it.

@jimmypuckett
Copy link
Contributor Author

OK, I have refactored the $index & $count as public & passed the Sequence object to the closure as we discussed. Also, I added a simple test.

Again, thanks for considering this.

@taylorotwell taylorotwell merged commit dc69b26 into laravel:8.x Jun 25, 2021
@jimmypuckett jimmypuckett deleted the passSequenceIndexToClosure branch December 1, 2021 01:55
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.

2 participants