dbrattli / Expression

Pragmatic functional programming for Python inspired by F#
https://expression.readthedocs.io
MIT License
424 stars 31 forks source link

Added bind to Seq #43

Closed paucarre closed 2 years ago

dbrattli commented 2 years ago

Thanks for the PR. Bind is called collect in this library. Is there any difference between the two?

paucarre commented 2 years ago

@dbrattli oh, I didn't expect collect to do that.

bind to me is the functional flat_map . I see that option and other types use bind as flat_map, which is the "monad method". To me it is very confusing that the "monad method" is called bind for some types and collect for others. Specifically, a Monad flat_map/bind should have the following signature:

    def bind(self, mapper: Callable[TSource, Monad[TResult]]) -> Monad[TResult]:

Nonetheless, I would say, having different names for the same "logic"/"idea" causes confusion more than anything.

collect to me has potentially other meanings like the ones in Akka streams

I can close the PR anyways.


Just as a side note, and apologies if I sound patronizing, only trying to (maybe) help! Once you have bind for all the types defined, you can just keep one single implementation of map that uses bind so you do not need to implement map for each specific type. Example:

    def map(self, mapper: Callable[TSource, TResult]) -> Monad[TResult]:
        lifted_function = lambda element: self.unit(mapper(element))
        return self.bind(lifted_function)

This map does work for any Monad type ( option, result, seq, ...) so long they have their own bind Here unit method is the method that creates the Monad given a raw type. This is the other method necessary to define a Monad. For an option it will return Some and for seq a sequence of one element, for result a Success of the element etc. This is quite a formal definition of Monad. For more info in Wikipedia, see "type converter" and "combinator".

As said, one does not need to implement Map as Map comes off of bind/flat_map. This is why a Monad ( bind/flat_map ) is a Functor ( only map ) but the other way around is not necessarily true.

dbrattli commented 2 years ago

I agree that this can be confusing. The seq module is ported from the F# Seq module where it called collect. Can you perhaps change your PR to add bind as an alias for collect (or the other way around)? I see they have added this alias to e.g FSharpPlus, so we're not the only one that thinks this can be confusing.

paucarre commented 2 years ago

@dbrattli I can add bind as alias for collect, is that ok for you?

paucarre commented 2 years ago

@dbrattli I added bind as an alias to collect but I also did a few changes, which I can revert if you tell me what you want. These changes are types in inputs and outputs. The inputs I used superclass ( Iterable instead of Seq) so that they are more "useful" (they span more cases). On the other hand, for the outputs I used the subclasses (Seq instead of Iterable) so that they are, again, more "useful", spanning more use-cases. This is related to covariance and contravariance of types in methods. I can revert back if you want.

dbrattli commented 2 years ago

@paucarre Thanks, this is very nice. For bind it's great to have the mapper return Iterable. But for seq as a module the idea is that the pure functions only works on Iterable (no wrap /unwrap, so should not return Seq as a class).

dbrattli commented 2 years ago

PS: Also seq testing usually need to list the result before you can compare since iterables are lazy.

paucarre commented 2 years ago

PS: Also seq testing usually need to list the result before you can compare since iterables are lazy.

Yep, that's already there with lists to load all the contents

paucarre commented 2 years ago

@dbrattli is that good to go now or are there anything left to change?

dbrattli commented 2 years ago

@paucarre Run tests locally with pytest fix any issues and commit. You also might want to run pre-commit run --all-files before submitting to make sure it builds and passes all checks. The branch is also out-of-date with main, so please merge main or rebase.

dbrattli commented 2 years ago

Closed because of inactivity