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

[5.2] Add pipe function to collection #13899

Merged
merged 1 commit into from
Jul 2, 2016
Merged

[5.2] Add pipe function to collection #13899

merged 1 commit into from
Jul 2, 2016

Conversation

freekmurze
Copy link
Contributor

This PR adds a pipe function to the Collection class that takes a callback, passes the collection into the callback, and returns the result.

@GrahamCampbell GrahamCampbell changed the title Add pipe function to collection [5.2] Add pipe function to collection Jun 6, 2016
@sisve
Copy link
Contributor

sisve commented Jun 7, 2016

Why?

  1. Aren't you just rewriting $callable($collection) with $collection->pipe($callable)? (+6 characters)
  2. Is the $callable function allowed to modify the collection?
  3. Could you give a use-case where this makes more sense to me, preferable at 06:50 in the morning? ;)

Other methods on Collection is typehinted, and we know that they either return the collection itself for method that supports chaining, or the requested result. The result from all methods are known; we can answer support entries with "a groupBy returns an array keyed by the value you return in the callable $groupBy".

What can we say about this method? "We cannot say what it returns, that depends on $callable. Can you show us the source of your $callable?"

@GrahamCampbell
Copy link
Member

Why?

What if you're returning a collection in a long chain of calls, then want to easily pipe. That's easier to read than nesting that inside a closure call?

@GrahamCampbell
Copy link
Member

Say:

User::where('love_collections', true)->get()->pipe($summary);

vs

$summary(User::where('love_collections', true)->get());

@sisve
Copy link
Contributor

sisve commented Jun 7, 2016

@GrahamCampbell I disagree that it's easier to read. I would argue that readability will be improved with intermediate variables.

$users = User::where('love_collections', true)->get();
$summaryResult = $summary($users)

If readability is the only argument; isn't it more important that all code bases include one, and only one, way to do it, instead of providing several different way of doing the same thing?

@GrahamCampbell
Copy link
Member

I would argue that readability will be improved with intermediate variables.

Well, we're not stopping you do that?

@GrahamCampbell
Copy link
Member

I'm in support of this PR. I think it's a really nice option for people to have.

@freekmurze
Copy link
Contributor Author

Though tap certainly feels a bit similar there's a big difference: the tap helper always the returns the $value given as the first argument. The pipe function from this PR can return anything.

@n7olkachev
Copy link
Contributor

Maybe we should also add tap method?

@adamwathan
Copy link
Contributor

adamwathan commented Jun 7, 2016

Here's an example from my collection book...

This pipeline takes a list of competition scores and ranks them, properly accounting for ties using Standard Competition Ranking

function rank_scores($scores)
{
    return collect($scores)
        ->sortByDesc('score')
        ->zip(range(1, $scores->count()))
        ->map(function ($scoreAndRank) {
            list($score, $rank) = $scoreAndRank;
            return array_merge($score, [
                'rank' => $rank
            ]);
        })
        ->groupBy('score')
        ->map(function ($tiedScores) {
            $lowestRank = $tiedScores->pluck('rank')->min();

            return $tiedScores->map(function ($rankedScore) use ($lowestRank) {
                return array_merge($rankedScore, [
                    'rank' => $lowestRank
                ]);
            });
        })
        ->collapse()
        ->sortBy('rank');
}

This is obviously hella not-expressive. If you wanted to break parts of this pipeline up into named functions, currently you'd need to use intermediate variables like this:

function rank_scores($scores)
{
    $rankedScores = assign_initial_rankings(collect($scores));
    $adjustedScores = adjust_rankings_for_ties($rankedScores);
    return $adjustedScores->sortBy('rank');
}

The intermediate variables IMO are completely unnecessary and just add noise. pipe let's you do this:

function rank_scores($scores)
{
    return collect($scores)
        ->pipe(function ($scores) {
            return assign_initial_rankings($scores);
        })
        ->pipe(function ($rankedScores) {
            return adjust_rankings_for_ties($rankedScores);
        })
        ->sortBy('rank');
}

...and even this:

function rank_scores($scores)
{
    return collect($scores)
        ->pipe('assign_initial_rankings')
        ->pipe('adjust_rankings_for_ties')
        ->sortBy('rank');
}

If you haven't developed an innate hatred for intermediate variables, I can see why it might not seem like an improvement. I hate variables :)

@lucasmichot
Copy link
Contributor

Can this not be achieved with a collection macro ?

@lucasmichot
Copy link
Contributor

Still I like the idea, but why wouldn't tap be in a trait so that we could also use it somewhere else - like in model ?

@sisve
Copy link
Contributor

sisve commented Jun 7, 2016

I'm unable to recall any other language that has a pipe method, but I've not used many languages anyway. However, the process of transforming every item on the list is what the map method does. Building a new list isn't really functionality that belongs to the original collection instance, in my opinion, thus I disagree that it should be an instance method on that collection. All this method does is some weird reversing-of-relationship in who-calls-who.

Also, I cannot see any way to type-hint it with anything else than "mixed", and removing any attempts at static code analysis.

I believe that this method, if added, should also be added to many other objects to keep some kind of consistency.

return $app->make('ControllerDispatcher')
  ->pipe($buildController)
  ->pipe($invokeAction)
  ->pipe($renderView)
  ->pipe($assignToResponse);

I see a nightmare in teaching new developers to debug an error in such pipes (Do we call these pipes?), without being able to tell them to dd() some intermediate variable.

I'm guessing you guys look forward to https://wiki.php.net/rfc/pipe-operator ?

@sebastiandedeyne
Copy link
Contributor

Also, I cannot see any way to type-hint it with anything else than "mixed", and removing any attempts at static code analysis.

I don't really see this as a fair counter argument. PHP is a dynamic language, we don't have to be able to statically analyse every bit of code. Also, the same applies to reduce, which I'd consider a 'classic' list method, and having a mixed return type doesn't decrease it's value.

@lucasmichot
Copy link
Contributor

lucasmichot commented Jun 11, 2016

@freekmurze, what about a Pipable interface and a Pipable trait instead ?

@ntzm
Copy link
Contributor

ntzm commented Jun 13, 2016

@lucasmichot There only needs to be an interface if multiple classes are going to implement it imo. This PR is only targeting collections.

@lucasmichot
Copy link
Contributor

@ntzm yes I could read the issue title.
And I am speaking of an interface AND a trait.

@ntzm
Copy link
Contributor

ntzm commented Jun 13, 2016

@lucasmichot I don't want to start an argument man, I'm just saying it's not really necessary for this PR (imo).

@lucasmichot
Copy link
Contributor

Sure @ntzm ;-) discussion can go on here laravel/ideas#112

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.

9 participants