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] Add Str::pipe & make Stringable tappable #36017

Merged
merged 2 commits into from
Jan 25, 2021
Merged

[8.x] Add Str::pipe & make Stringable tappable #36017

merged 2 commits into from
Jan 25, 2021

Conversation

binotaliu
Copy link
Contributor

@binotaliu binotaliu commented Jan 24, 2021

This PR add a simple pipe method (feel free to suggest other names) that similar to Collection::transform.
I have considered to name it transform, but transform mutates itself.

Usage example:

Str
  ::of('foo > bar')
  ->pipe('htmlentities') // callable
  ->pipe(fn ($str) => md5($str)); // closure

While Stringable is already macroable, often time we only need to run something once without need to register it into global.

I also made Stringable tappable:

Str
  ::of('foo > bar')
  ->pipe('htmlentities')
  ->tap(fn ($str) => Log::info('Origin str: ' . $str))
  ->pipe('md5');

@antonkomarev
Copy link
Contributor

Maybe just make it tappable is enough?

@binotaliu
Copy link
Contributor Author

binotaliu commented Jan 24, 2021

@antonkomarev tap() ignores the return value of the callback.


How about naming it pipe()? I just renamed it.


Edited:

A same example write without this will be like:

$str = 'foo > bar';
$str = htmlentities($str)
Log::info('Origin str:' . $str);
$str = md5($str);

This small helper add the ability to add huge flexibility while we can have the fluent Stringable already does.

@binotaliu binotaliu changed the title [8.x] add Str::do & make Stringable tappable [8.x] add Str::pipe & make Stringable tappable Jan 24, 2021
@GrahamCampbell GrahamCampbell changed the title [8.x] add Str::pipe & make Stringable tappable [8.x] Add Str::pipe & make Stringable tappable Jan 24, 2021
@taylorotwell taylorotwell merged commit 7f1138e into laravel:8.x Jan 25, 2021
@binotaliu binotaliu deleted the feature/tap-do-stringable branch January 25, 2021 04:51
@JosephSilber
Copy link
Member

JosephSilber commented Mar 1, 2021

Again I cry 😢

Why does this work differently than every other pipe?

pipe should do one thing: pipe the instance into the callback, and return what the callback returns.

Wrapping the returned value in a new Stringable is not within the scope of pipe. No other pipe works like this 😞


There very well may be a need for the method in this PR, but it should be named something else. Maybe map is a good name. But pipe should be reserved for simple piping.

@binotaliu
Copy link
Contributor Author

Hi @JosephSilber , I agree that the naming is misleading.
Can you provide some examples of "other pipes" ?

@binotaliu
Copy link
Contributor Author

I agree that using the word pipe would make people confused with the pipe operator. From my point, the Stringable#pipe method works inside a Stringable. If you want to return a value other than string, it's not what we expected to do in the Stringable as its main purpose is to have a fluent way to perform actions on a string.

@JosephSilber
Copy link
Member

Can you provide some examples of "other pipes"?

The Collection's pipe would be the simplest example:

/**
* Pass the collection to the given callback and return the result.
*
* @param callable $callback
* @return mixed
*/
public function pipe(callable $callback)
{
return $callback($this);
}

No one ever thought of forcing users to return an array, and wrapping the returned value in a Collection. That's not pipe's job.

@JosephSilber
Copy link
Member

JosephSilber commented Mar 1, 2021

Actually, I just realized that this is a simple map (and should pass the inner string to the callback).

That way Stringable becomes a functor that you can map over easily.

So yes, map would be the correct name for this.

@binotaliu
Copy link
Contributor Author

@JosephSilber I don't know if renaming current method is a good idea. But we can introduce break change in next major version, to stop wrapping return values in Stringable.

We can still maintain a BC compatibility by wrapping primitive string:

public function pipe(callable $callback)
{
    $piped = call_user_func($callback, $this);
    return is_string($piped) ? new static($piped) : $piped;
}

I know this solution still does extra things that shouldn't be done in pipe.

@JosephSilber
Copy link
Member

JosephSilber commented Mar 16, 2021

Actually, I just realized that this is a simple map (and should pass the inner string to the callback).

And now I realized: the way it is currently implemented it's not a map, since with map it would have to be unwrapped before being passed to the callback, which it isn't (could we?)

But we can introduce break change in next major version, to stop wrapping return values in Stringable.

Correct, though we should still keep around a version of this method, under a different name.

We can still maintain a BC compatibility by wrapping primitive string

That would break BC for callbacks that return an object that can be cast to a string.

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