Fix SemigroupK[Resource[F, *]].combineK

[BalmungSan]

Changing the behavior of SemigroupK[Resource[F, *]].combineK to be consistent with MonadError[Resource[F, *], E].orElse

Fixes #949

---

I am surprised I didn't have to change any test (this is probably a sign that there was no reason for the previous behavior).
Should I add some tests for the current behavior?

BTW, I confirmed it worked as expected using the snippet posted in the original issue.

[BalmungSan]

It seems my implementation is not binary compatible.
I could preserve the old class, and still require a SemigroupK for F (even if unneeded).
But still, the new implementation requires a MonadError for F instead of just a Monad... so, it wouldn't matter.

[djspiewak]

Ah! So the trick to appease the MiMa gods here is to take the old method, remove the implicit modifier and make it private[effect]. It compiles to the same bytecode as what you have there now, but any call sites will recompile to use the new (correct) signature. It's definitely worth doing this, even though the old one was kind of broken and dumb, just to make sure we really hold up the "binary compatible" guarantee. :-)

[BalmungSan]

@djspiewak oh good to know, I will make the change.
However, even if it is binary compatible. It is acceptable that using a different jar (that was supposed to be binary compatible) will produce a different runtime behavior? Or in this case, are we considering this a bug fix?

BTW, what do you think about the tests?

[djspiewak]

However, even if it is binary compatible. It is acceptable that using a different jar (that was supposed to be binary compatible) will produce a different runtime behavior? Or in this case, are we considering this a bug fix?

Well, it's still going to produce the same runtime behavior. You can't call the new implementation from the old signature, so you just have to keep the old implementation around, but hidden behind the private[effect] signature. So that part should be okay.

If we were in the situation you described, I definitely would need to be sure it was a bugfix. :-)

BTW, what do you think about the tests?

I think it's definitely worth adding some tests. It doesn't entirely surprise me that the behavior was previously untested, since I feel like anyone writing such a test would realize how silly the semantics were.