typelevel / cats

Lightweight, modular, and extensible library for functional programming.
https://typelevel.org/cats/
Other
5.26k stars 1.21k forks source link

Add `mapOrKeep` to Functor #4582

Closed jozic closed 6 months ago

jozic commented 7 months ago

I've found myself (and others) repeating this over and over, and it seems as primitive as void or as, so would be nice to have it here. If this looks fine, i can also add flatMapOrKeep to Monad in another PR

satorg commented 7 months ago

Thank you for the contribution! It seems that the suggested function is supposed to shorten expressions like this one below:

fa.map {
 case `cond_a1` => calc_a1
 ...
 case `cond_aN` => calc_aN
 case default => default
}

into something like this:

fa.alter {
 case `cond_a1` => calc_a1
 ...
 case `cond_aN` => calc_aN
}

So basically it should help to avoid the "default" case, would it be an accurate assumption?

I feel that since it is basically a map but with an additional feature, then its name would better look like mapOrSomething. Maybe mapOrDefault?

Sorry for bikeshedding but I feel that names like alter would be difficult to discover – naturally, there's no alteration implied.

jozic commented 7 months ago

So basically it should help to avoid the "default" case, would it be an accurate assumption?

yep, that's correct

I feel that since it is basically a map but with an additional feature, then its name would better look like mapOrSomething. Maybe mapOrDefault? Sorry for bikeshedding but I feel that names like alter would be difficult to discover – naturally, there's no alteration implied.

Don't be sorry :) you concern makes sense. I also wanted to use something like mapSomething, but my main concern with that is that it's more limited than map because of A1 >: A constraint. And given, that it seems to me that it's not really mapping A, but rather transforming (or altering) it. So that's why to me alter seems like a good name here, but it indeed can be harder to discover.
As an alternative, how about mapWhen? it's gonna read as assert(l.mapWhen { case i if i % 2 == 0 => i + 1 } === l.map(i => if (i % 2 == 0) i + 1 else i))

jducoeur commented 7 months ago

The name map feels wrong to me here. I expect map to be total, and this isn't.

My gut says that the correct name here is collectOrKeep -- in general, I expect PartialFunction to go with collect.

jozic commented 7 months ago

Just for the record: my original name was alter, then the rest of the folks who kindly reviewed this more or less agreed on mapOrKeep. Which also makes sense to me, but I don't agree this has something in common with collect (except using PF). Oscar earlier pointed out that we don't want someone to confuse this new method with some version of collect, which I agree with.

danicheg commented 7 months ago

Continuing bikeshedding on naming, there were several inquiries to consider the collect* name (collectOrKeep / collectOrElse) from the community. In my humble opinion, if we are about to add the new method to Functor — it's much more likely a map operation and therefore mapOrKeep is fine. But if one adds that method to FunctorFilter, I would agree that collectOrKeep better fits. Because FunctorFilter is seemingly for collections, but Functor is a more general abstraction.

jozic commented 7 months ago

@danicheg Do you want to re-review this again?

jozic commented 7 months ago

@johnynek could you please quickly re-review this? Thank you!

jozic commented 6 months ago

@danicheg @johnynek A gentle ping. Thank you.

Is there anyone else who can review this?

johnynek commented 6 months ago

Thank you for the PR