scalawithcats / scala-with-cats

Source code for Scala with Cats
http://underscore.io/books/scala-with-cats
398 stars 133 forks source link

content of case study :CRDTs got unexpected result #206

Open chenghsienwen opened 3 years ago

chenghsienwen commented 3 years ago

this is about the result of case study is unexpected my test of unexpected result https://github.com/chenghsienwen/AmmSandbox/blob/master/cats-execise-crdt4.sc

expected result: https://github.com/chenghsienwen/AmmSandbox/blob/master/cats-execise-crdt3.sc

does any one get correct result on "K.5 Abstracting a Key Value Store"?

Thank you

RyotaBannai commented 3 years ago

I faced the same issue as @chenghsienwen mentioned, the BoundedSemiLattice |+| method should be called, but the default combine method from Monoid is called instead, assuming that scala compiler selects narrower one to apply, I'm not sure how to get around.

nathanmbrown commented 1 year ago

I just ran into this also. The issue is that, for the merge to work correctly as coded with the abstracted GCounter instance, the Int-specific BoundedSemiLattice instance needs to be implicitly available when it is constructed, so that the combine operation of the Monoid/Semigroup for the KVS in the merge has it available when combining the Maps later. If you don't import it, then it will use catsKernelStdGroupForInt instead to merge items which just adds them instead, breaking the merge logic. See the screenshot below that shows where it is going in: image The problem is, if you do make it implicitly available at that point then it is preferred over the catsKernelStdGroupForInt when calling the total method on the GCounter later. See screenshot below. One way to remedy is to then explicitly pass catsKernelStdGroupForInt when calling total but that is not great. (shown as green in screenshot) image It would be good to have some guidance from the authors on best practices on how to avoid or work around these issues - this sort of thing would make me nervous to start using cats in production code as these sorts of issues are pretty opaque and hard to understand! cc: @noelwelsh / @davegurnell

noelwelsh commented 1 year ago

This is an issue with this case study (and why I now think it's not a good example of using type classes): it requires two different type class instances on the same type to function correctly (one is the semilattice, which should behave like max and one is the monoid that should behave like +).

It's not something that shows up in usual practice using type classes, and going into this subtlety isn't really appropriate for the audience Scala with Cats is aimed at.

https://creativescala.github.io/case-study-parser/index.html is an example of new material that's in development, which I think is better.