evolution-gaming / cats-helper

Helpers for cats & cats-effect
MIT License
50 stars 17 forks source link

feat: add `combineWith` method to Mdc #162

Closed kpodsiad closed 2 years ago

kpodsiad commented 2 years ago

Tiny amount of sugar for folks who like methods.

t3hnar commented 2 years ago

@kpodsiad why not implement Semigroup[Mdc] instead? :)

kpodsiad commented 2 years ago

@t3hnar sorry, could you rephrase?

t3hnar commented 2 years ago

@t3hnar sorry, could you rephrase?

if you implement implicit instance of Semigroup for Mdc - then you will have all those helpers methods out of the box

object Mdc { 
  …
  implicit val semigroupMdc: Semigroup[Mdc] = ???
  …
}
t3hnar commented 2 years ago

and I just checked - it has already Semigroup implemented, it should be Monoid though…

    implicit final val mdcSemigroup: Semigroup[Mdc] = Semigroup.instance {
      case (Empty, right) => right
      case (left, Empty) => left
      case (Context(v1), Context(v2)) => Context(v1 ++ v2)
    }    

then the question - why do you need this method at all? if import cats.syntax.all._ should add proposed method? :)

kpodsiad commented 2 years ago

Now I understand your original answer, but I also see that you answered it ;)

why do you need this method at all? if import cats.syntax.all._ should add proposed method?

It's a simple matter of better user experience and leveraging method instead extension functions, which are easier to discover. Of course you can import cats.syntax.all._ like you wrote but it pollutes namespace with, most of the time, useless methods unfortunately.

That being said, I've created this PR mostly because I was using the later approach and I wanted sth more convenient. However, through time my need to combine mdcs was gone and I don't need it anymore, so I don't want to advocate too much on it ;)

t3hnar commented 2 years ago

ok :) I'm closing it then.