snoyberg / classy-prelude

A typeclass-based Prelude.
108 stars 15 forks source link

I'd like `elem` to work for NonEmpty. #93

Closed bitemyapp closed 9 years ago

bitemyapp commented 9 years ago

screenshot from 2015-01-23 11 51 58

had to hide classy-prelude elem, then import Data.Foldable elem.

gregwebs commented 9 years ago

This would be a mono-traversable issue. The problem right now is that elem requires IsSequence and IsSequence requires fromList (which would be partial for a NonEmpty).

@snoyberg can we move elem to be a member of MonoFoldable instead?

bitemyapp commented 9 years ago

@gregwebs apologies for the mis-filed issue. I am still working on my first pot of coffee today.

snoyberg commented 9 years ago

That move makes sense to me @gregwebs.I won't have time to do this myself for a few days, but if someone sends a PR I'll be happy to merge.

snoyberg commented 9 years ago

Actually, turns out that we can't move elem into MonoFoldable, since it requires an Eq constraint. That's why we currently have a EqSequence typeclass. We could add a similar EqMonoFoldable typeclass, but I'd rather not do so. Another option is just providing:

oelem x = oany (== x)
gregwebs commented 9 years ago

From Data.Foldable:

-- | Does the element occur in the structure?
elem :: (Foldable t, Eq a) => a -> t a -> Bool
elem = any . (==)

So yes, that is the approach we should take. The only question is how to deal with breakage from this.

snoyberg commented 9 years ago

In mono-traversable, there's no need for breakage. Following the current naming convention, we'd name the new function oelem, so it wouldn't conflict. Then the question is whether we should change the export in classy-prelude. But given that the new function will just be a generalized version of the current one, it should be just a minor bump.

gregwebs commented 9 years ago

Shouldn't we remove elem from EqSequence? We can start a deprecation cycle though.

snoyberg commented 9 years ago

I'm not sure we should. It's possible in some cases for that function to be more optimized than what we'll define as oelem, though theoretically rewrite rules could address those cases.

gregwebs commented 9 years ago

I don't think having both an oelem and an elem is a good idea.

only stripPrefix and stripSuffix actually need to be a IsSequence. And all the instances seem to use the defaults for the rest of the functions. So if you want specialized implementations, EqMonoFoldable seems to be a viable approach.

Particularly if we re-think the stripPrefix and stripSuffix implementations.

snoyberg commented 9 years ago

OK let's go with the deprecation cycle. I'll look into the stripPrefix stuff in more detail when I'm at my computer.

gregwebs commented 9 years ago

Hmm, I don't think there is any way to change the nature of stripPrefix and stripSuffix: they need to create a new data structure and thus need fromList.

gregwebs commented 9 years ago

My vote is to put as much as possible into a new MonoEqFoldable typeclass. That doesn't leave EqSequence with much, but there are some more functions that could be added from Data.List. Some potential candidates are Set operations such as delete, intersection, union, and the Set creator nub.

snoyberg commented 9 years ago

@gregwebs I think you have a clearer idea of what this would look like than I do. Do you want to take a stab at this?

gregwebs commented 9 years ago

PR is here: https://github.com/snoyberg/mono-traversable/pull/46

On the classy-prelude side we can version bound mono-traversable to do transparent renames avoid any deprecation warnings.

gregwebs commented 9 years ago

@snoyberg can you give me permission to upload mono-traversable? I pushed the 0.8.0 tag if you want to upload it yourself.

snoyberg commented 9 years ago

Added.

gregwebs commented 9 years ago

i believe this is fixed and on hackage now