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

First implementation of fluent interface #78

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

SamMousa
Copy link
Contributor

First draft of #45

@SamMousa
Copy link
Contributor Author

Some ideas that could be implemented:

  • Early error checking; since we don't use generators we can often validate params before iteration starts.
  • Have toIter() return FluentIterator or add toFluentIterator()
  • Add iterator creators like range() and repeat() as static constructors

@jvasseur
Copy link
Contributor

I'm 👎 here, having a fluent interface would be annoying when you need to mix function from this package with your own functions (or functions from other packages).

I think instead we should provide curried versions of the functions and a pipe/flow function to allow chaining any list of function.

https://medium.com/making-internets/why-using-chain-is-a-mistake-9bc1f80d51ba explains in details (in javascript but some problems apply to PHP to) why such an interface comes with lots of problems.

@SamMousa
Copy link
Contributor Author

@jvasseur can you be more specific?

I'm -1 here, having a fluent interface would be annoying when you need to mix function from this package with your own functions (or functions from other packages).

Can you give an example, things like apply, or map should allow you to mix this with your own or third party functions easily. Also the intermediate object itself is an iterable, so you could pass it to anything that takes an iterable.

I think instead we should provide curried versions of the functions and a pipe / flow function to allow chaining any list of function.

Could you give a syntax example of how you envision this? I'm definitely open to suggestions.

https://medium.com/making-internets/why-using-chain-is-a-mistake-9bc1f80d51ba explains in details (in javascript but some problems apply to PHP to) why such an interface comes with lots of problems.

Could you specify which problems you think apply to PHP?

For me this is just a small wrapper that allows me to create more readable code without taking away any of the flexibility, I'm not proposing this implementation replaces the functional approach...

@ragboyjr
Copy link
Contributor

@SamMousa I tend to agree with @jvasseur , I think the only way you could do fluent collection like stuff would be if you provided macros like laravel's collection package to allow patching in, but that comes at the cost of discoverability.

The issue he's referring to is, let's say you built your own function like chunkBy(callable $predicate) with the fluent version, you have no way of accessing that function in the chain. You'd have to provide the ability to like monkey patch the Iter class to allow arbitrary additions.

The other minor issue is that any new functions/changes need to be kept in sync and tested

Regrading iter vs functional syntax, here are some examples:

https://github.com/krakphp/fn - this is an example of what you can do with curried functions and compose/pipe chains.

and here's a syntax comparison (as shown in the article as well)

<?php

// imperative
$values = [1,2,3];
$values = map(function() {}, $values);
$values = filter(function() {}, $values);

// imperative chained
$values = filter(
    function() {}, 
    map(
        function() {}, 
        [1,2,3]
    )
);

// functional curried w/ compose
$values = compose(
    filter(function() {}), 
    map(function() {})
)([1,2,3]);

// functional curried w/ pipe
$values = pipe(
    map(function() {}),
    filter(function() {})
)([1,2,3]);


// fluent chaining
$values = iter([1,2,3])
    ->map(function() {})
    ->filter(function() {});

@jvasseur
Copy link
Contributor

@jvasseur can you be more specific?

I'm -1 here, having a fluent interface would be annoying when you need to mix function from this package with your own functions (or functions from other packages).

Can you give an example, things like apply, or map should allow you to mix this with your own or third party functions easily. Also the intermediate object itself is an iterable, so you could pass it to anything that takes an iterable.

What I mean is if you have your function myFunction that you want to put in the chain like this:

$fluent
    ->filter(...)
    ->myFunction()
    ->map(...)

@SamMousa
Copy link
Contributor Author

@ragboyjr thanks for the examples!

I'm not sure I fully agree with the reasoning though.

The issue he's referring to is, let's say you built your own function like chunkBy(callable $predicate) with the fluent version, you have no way of accessing that function in the chain. You'd have to provide the ability to like monkey patch the Iter class to allow arbitrary additions.

Because the fluent version is iterable itself you can just pass it into your function.

$fluent = new Fluent(['a','b']);

$result = chunkBy($fluent->map(...)->slice(1, 6))

Admittedly, you lose the syntactic sugar a bit.
It would however by simple to extend the syntax like this:

$fluent = new Fluent(['a','b']);
$fluent->map(...)
    ->slice(1, 6)
    ->via($chunkBy)
    ->map(...);

The issue with PHP is that you cannot reference a function naturally, unless it is a closure stored in a variable.

The other minor issue is that any new functions/changes need to be kept in sync and tested

This is true, but this is also the case for curried functions, as you can see in the library you mentioned, it comes with both curried and uncurried functions as well as constants so you can reference those functions.

One thing PHP has that the other languages don't is that it allows objects to be iterable. So we have no need to distinguish between intermediate stuff and final stuff.

@SamMousa
Copy link
Contributor Author

To extend the example assuming you don't have a clean reference to your function:

$fluent->map(...)
    ->via(function(Fluent $fluent): iterable {
        return chunkBy($fluent);
    })
    ->slice(1, 6);

This ticks all my boxes:

  • Readable
  • Full IDE assistance since we don't use strings to refer to functions.
  • Use third party functions even if they are not in the library.
  • Doesn't use a generator so preserves rewindability.

I understand the arguments given against this, but PHP is not a functional programming language. It has drawbacks but also advantages. The advantage here is iterable, and that's what I'm trying to exploit to create this syntax that is in my opinion more readable than the alternatives.

@ragboyjr
Copy link
Contributor

In the lib I mentioned, the curried functions are automatically generated. So it's an automated process that uses php parser to generate the code. Which is something you could do with a fluent interface as well I supposed.

Regarding that via example, that's not so bad, if you're functions were curried, then it should work fine:

<?php

namespace Acme\Util;

const chunkBy = chunkBy::class;
function chunk($args) {
  return function($value) use ($args) {};
}
use Acme\Util;

$fluent->map()->via(Util\chunk(args));

@SamMousa
Copy link
Contributor Author

In the lib I mentioned, the curried functions are automatically generated.

We could even omit the functions and just use __call(), and then document them via PHPDoc.
For me both are viable, also adding new functions to this library doesn't happen that often so even duplicating is not that much of an effort.

@ragboyjr
Copy link
Contributor

@SamMousa
Copy link
Contributor Author

Yeah, they do it for static constructors.
Anyway, I'm open to any solution for that problem, but still we need to decide if this fluent interface is something we want as an addition to the current functional interface.

I think I've given counters to most arguments, are there more issues with this solution?

*/
public function via(callable $callable): self
{
return new static(call_user_func($callable, $this));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can provide an interface that allows passing extra args:

public function via(callable $callable, ...$args): self
{
    return new static($callable($this, ...$args));
}

this would remove the need to create closures as long as you need to pass the iterator as the first argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of that as well, we could even make it dynamic like this:

public function via(callable $callable, array $before = [], array $after =[]): self
{
    return new static(call_user_func_array($callable, array_merge($before, [$this], $after)));
}

Although that might be a bit too much.

Note that in PHP you generally don't start with a callable to begin with, so a closure is often required regardless of the support for this.

Copy link
Contributor

@jvasseur jvasseur Mar 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in PHP you generally don't start with a callable to begin with, so a closure is often required regardless of the support for this.

Why? All functions can be used as callable by passing the fully qualified name as a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvasseur true, but that's not good in my opinion.. It removes all static checks or ide support.
I'd rather wrap my built-in function in a closure than use a quoted string to reference a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I propose leaving it as it is now.

@SamMousa
Copy link
Contributor Author

SamMousa commented Apr 3, 2019

@nikic do you have an opinion?

Copy link
Owner

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty nice, I think we should add it.

README.md Show resolved Hide resolved
src/FluentIterator.php Outdated Show resolved Hide resolved
src/FluentIterator.php Outdated Show resolved Hide resolved
src/FluentIterator.php Outdated Show resolved Hide resolved
src/FluentIterator.php Outdated Show resolved Hide resolved
src/FluentIterator.php Outdated Show resolved Hide resolved
src/FluentIterator.php Outdated Show resolved Hide resolved
src/FluentIterator.php Outdated Show resolved Hide resolved
src/FluentIterator.php Outdated Show resolved Hide resolved
src/FluentIterator.php Outdated Show resolved Hide resolved
@theofidry
Copy link

@SamMousa @nikic what would it take to finish this?

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.

5 participants