-
Notifications
You must be signed in to change notification settings - Fork 183
Add filterM #120
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
Add filterM #120
Conversation
Thus far, this proposal has gotten good reception on the libraries list.
Have you benchmarked and showed that it's faster than filterM . toList
. In the case of bytestring we rejected adding a patch like this because using rewrite rules we got the same performance without growing the API.
Oh, you mean filterMSeq f = fmap fromList . filterM f . toList
? I guess that's an option, and probably a good implementation, but it's a lot less convenient. There seems to be a fairly broad agreement that making it more convenient to use Data.Sequence
is a good thing. However, that implementation does reveal a nice property: filterMSeq
is fundamentally an applicative filter producing a Seq
; it's perfectly happy to take any Foldable
as input.
The bigger question here is: should we add applicative versions of every function to the containers API? There's an argument to be made for it. You might want to perform some effect when doing e.g. an alter
. The obvious downside is huge amount of code duplication and a much harder to understand (and less composable) API. That's why we've been pushing back on this so far. Our functional container APIs are already much bigger than their imperative siblings. I'm worried we'll end up with APIs so big that their hard to use.
There's been a big trend to move away from lists as the only container type that gets a lot of good functions, and I personally think that's a good thing. Making people roll their own functions to use containers
makes them more likely to just give up and use lists or vectors instead, whether or not those are the most appropriate. It is true that the API for IntMap
is a bit intimidating (what, for example, can updateWithKey
do that update
cannot, aside from weird things with broken Eq
instances?) but Seq
is another story.
Appologies if this is too tangential: I haven't seen a proposal but is there any reason why filterM
from base couldn't be changed to:
filterM :: (Applicative m, Foldable t) => (a -> m Bool) -> t a -> m [a]
Then not every data structure would have to provide its own implementation of these functions since it would just use the Foldable instance, which Sequence already has. This is obviously a much larger change, but was wondering if this has been considered.
Edit: duh, this doesn't have the same type as the filterM in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation looks kind of silly, since it suspends the insertions till the end. A monadic version can build the result sequence eagerly as it goes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes based on @treeowl's review:
This implementation looks kind of silly, since it suspends the insertions till the end. A monadic version can build the result sequence eagerly as it goes.
Thus far, this proposal has gotten good reception on the libraries
list.