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

Enable fluent interface for streams #109

Closed
joshdifabio opened this issue Mar 18, 2017 · 12 comments
Closed

Enable fluent interface for streams #109

joshdifabio opened this issue Mar 18, 2017 · 12 comments

Comments

@joshdifabio
Copy link
Contributor

joshdifabio commented Mar 18, 2017

The stream functions suffer from the same problems as PHP's array functions; they're awkward to chain.

$output = filter(map(filter($input, $filterFn), $mapFn), $otherFilterFn);

This would be preferable imo:

$output = $input
    ->apply(filter($filterFn))
    ->apply(map($mapFn))
    ->apply(filter($otherFilterFn));

Edit: Inspired by apache beam.

@kelunik
Copy link
Member

kelunik commented Mar 18, 2017

Another solution is to just write:

$stream = Stream\filter($stream, $filterFunctor);
$stream = Stream\map($stream, $mapFunctor);
$stream = Stream\filter($stream, $otherFilterFunctor);

But I agree, chaining can be a bit nicer to read. Unfortunately such functionality can only be implemented by adding the to be chained methods to each and every implementation, mixing responsibilities.

@kelunik
Copy link
Member

kelunik commented Mar 18, 2017

What could be done is using a wrapper, e.g.

$output = transform($stream)
    ->filter(...)
    ->map(...);

where transform wraps the stream in another stream that provides these methods.

@joshdifabio
Copy link
Contributor Author

Unfortunately such functionality can only be implemented by adding the to be chained methods to each and every implementation, mixing responsibilities.

My suggestion is to only add a single apply() method to the implementations, not map(), filter(), flatMap() etc.

apply() would take a Transform object as an argument; map(), filter(), etc. would return Transform objects.

@kelunik
Copy link
Member

kelunik commented Mar 18, 2017

How would the API of a Transform object and the implementation of apply look like?

@joshdifabio
Copy link
Contributor Author

That's the tricky part!

@kelunik
Copy link
Member

kelunik commented Mar 18, 2017

@joshdifabio
Copy link
Contributor Author

Ah, you already have an async iterator!

interface Transform
{
    function apply($value): Iterator;
}

So the Stream::apply() function wouldn't be too difficult to implement I suppose.

@kelunik
Copy link
Member

kelunik commented Mar 18, 2017

@joshdifabio We have the following options:

$stream = $stream->apply($a)->apply($b);
$stream = transform($stream)->apply($a)->apply($b);
$stream = transform($stream)->filter($a)->map($b);

The first one requires an additional method and violates single responsibility. The second one allows custom transforms, the third one is more expressive, but doesn't allow custom transforms.

@joshdifabio
Copy link
Contributor Author

joshdifabio commented Mar 18, 2017

I like the first option the most. The second is okay too but it hides the functionality from users since the Stream interface wouldn't expose it. I think the third option is a bit too opinionated about which transform functions are worthy of being included in whatever object transform() returns.

One of the nice things about the apply($transform) pattern is that you can do things like composite transforms, and domain-specific objects can return domain-specific transforms to clients, for example:

$stream = $stream->apply(chunkStream())
                 ->apply($gzipAlgo->compressStream())
                 ->apply($cryptAlgo->encryptStream());

@kelunik
Copy link
Member

kelunik commented Apr 27, 2017

I guess we should move this issue to amphp/byte-stream.

@kelunik
Copy link
Member

kelunik commented May 3, 2017

This applies to both Amp\Iterator and amphp/byte-stream.

@kelunik kelunik modified the milestone: v2.x May 4, 2017
kelunik added a commit that referenced this issue May 21, 2017
This allows chaining transformations. Closes #109.
@kelunik
Copy link
Member

kelunik commented May 22, 2017

Closing for now, we might revisit it at a later time. It can always be added, as the feature is self-contained. See #136.

@kelunik kelunik closed this as completed May 22, 2017
staabm pushed a commit to staabm/amp that referenced this issue Nov 29, 2017
Add warning about locally closed resources being undefined behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants