Add mapOrKeep to Functor

[jozic]

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]

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]

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]

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]

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.

[satorg]

Although it is a valid test case - no doubt in that, but it is not really necessary to invent a custom test logic there:

    forAll { (l: List[Int], o: Option[Int], m: Map[String, Int], pf: PartialFunction[Int, Number]) =>
      assertEquals(l.mapOrKeep(pf), l.map(x => pf.applyOrElse(x, _ => x: Integer))
      // same for `o: Option[Int]` and `m: Map[String, Int]`
    }

It would be way clearer and more generic.
Also:

1. PartialFunction[Int, Number] so it check that the A1 >: A constraint holds too.
2. I would probably go with assertEquals instead of just assert because the former provides better reporting in a case of test failures.

That said, I feel that this equivalence could be just a part of the FunctorLaws:

Functor[A].mapOrKeep(fa)(pf) <-> Functor[A].map(fa)(a => pf.applyOrElse(a, _ => a: A1)

If we could add such a rule to the laws, we would not need to check for special cases individually – all possible implementations would be got checked automatically.

[jozic]

@satorg i guess I'm missing something again, but Int is not a subtype of Number
type arguments [Int,Number] do not conform to method m's type parameter bounds [A,A1 >: A]

[satorg]

Correct, Int is definitely not, but Integer is.

[jozic]

But there are no generators for Integer. As I mentioned in another comment, I didn't find any examples of A and A1 with A1 >: A

[satorg]

Well, I thought that since there's a generator for Int then it wouldn't be too difficult to get one for Integer via boxing.

But on the second thought, maybe it's not really worth the hassle – perhaps PartialFunction[Int, Int] should be just enough for this sort of test.

Besides, if some method needs tests for syntax correctness (which may also include type correspondence/variance testing), then the SyntaxSuite could be a better bet for that.

cats/tests/shared/src/test/scala/cats/tests/SyntaxSuite.scala

Line 339 in 47fbad7

def testFunctor[F[_]: Functor, A, B]: Unit = {

[jozic]

given that we agreed to use PF[A, A] in the law test, seems like we should be covered by it, and it also uses syntax

[danicheg]

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]

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

[satorg]

Looks great to me, thanks!

[jozic]

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

[jozic]

@danicheg @johnynek
A gentle ping. Thank you.

Is there anyone else who can review this?

[johnynek]

Thank you for the PR

[joroKr21]

I thought we could use mapConserve for List, but it works only for AnyRef.
Ideally collections would avoid allocations when we keep.