haskell / containers

Assorted concrete container types
https://hackage.haskell.org/package/containers
315 stars 178 forks source link

Add filterKeys for Map and IntMap #972

Open flip111 opened 1 year ago

flip111 commented 1 year ago

https://github.com/haskell/containers/issues/866

treeowl commented 1 year ago

Please don't include your Stack files. While I won't require it, I would prefer to see validity tests and also property tests (with validity tests) added both for filterKeys and filterWithKey. Also, CI is failing.

flip111 commented 1 year ago

Hello. The stack files i committed accidentally in my last commit. I will try to fix the test again

meooow25 commented 1 year ago

We have

filter p m = filterWithKey (\_ x -> p x) m

So why not

filterKeys p m = filterWithKey (\k _ -> p k) m

instead of duplicating the definition of filterWithKey?

konsumlamm commented 9 months ago

I don't see the advantage of this over just using filterWithKey?

flip111 commented 9 months ago

It's the equivalent of filter but then for keys

konsumlamm commented 9 months ago

Sure, I know what it does, but why should we add it, when we can just use filterWithKey, which isn't any slower, instead?

flip111 commented 9 months ago

I can't speak for the original poster @emilypi but for my point of view is that haskell libraries have many convenience functions that potentially can be expressed in terms of extra functions. With filterWithKey you have an additional argument that is not used (when using it like filterKeys).

emilypi commented 9 months ago

@flip111 that's exactly it. filterKeys is something many people do, and so we should optimize for it. The point is convenience. It's very low cost to add it, and i would argue it meets my personal Fairbarn threshold due to the fact that every other container library in existence features it.

konsumlamm commented 9 months ago

every other container library in existence features it.

Really? I don't think I've ever seen it anywhere, could you give some examples?

emilypi commented 9 months ago

Some examples:

There are many more examples, but I don't want to spend too much time on it. I don't see a valid argument against this other than "why not use filterWithKey", which has been addressed, and is largely a subjective for both sides. I've written this function alot, and I'd like to stop the repetition 😄

PPKFS commented 9 months ago

This seems like something that:

Melvar commented 9 months ago

I wanted this function a couple weeks ago. Of course I just used filterWithKey, but I had to check for the absence of filterKeys first.

flip111 commented 6 months ago

@treeowl @emilypi any more comments? I made some changes after your review.

emilypi commented 6 months ago

Looks fine to me, but it's up to @treeowl

flip111 commented 5 months ago

@treeowl could you take a look please? Is there anything i can do or something that needs to be done?