Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add filterM #120

wants to merge 1 commit into from

Conversation

treeowl
Copy link
Contributor

@treeowl treeowl commented Dec 30, 2014

Thus far, this proposal has gotten good reception on the libraries
list.

Thus far, this proposal has gotten good reception on the libraries
list.
@tibbe
Copy link
Member

tibbe commented Dec 30, 2014

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.

@treeowl
Copy link
Contributor Author

treeowl commented Dec 30, 2014

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.

@tibbe
Copy link
Member

tibbe commented Dec 30, 2014

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.

@treeowl
Copy link
Contributor Author

treeowl commented Dec 30, 2014

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.

@m-renaud
Copy link
Contributor

m-renaud commented Dec 22, 2017

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.

Copy link
Contributor Author

@treeowl treeowl left a 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.

Copy link
Member

@sjakobi sjakobi left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants