-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests for all derived instances #516
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Thanks so much!!
case LeafI(i) => Right(i) | ||
} | ||
|
||
enum EnumK1[A]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this meant to be Covariant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it doesn't matter
import scala.quoted.* | ||
|
||
/** An opinionated stack of traits to improve consistency and reduce boilerplate in Kittens tests. Note that unlike the | ||
* corresponding CatsSuite in the Cat project, this trait does not mix in any instances. | ||
*/ | ||
abstract class KittensSuite extends KittensSuite.WithoutEq, TestEqInstances |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these instances used for? I thought we were defining explicit instances for everything in ADTS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the EqInstances
separately to be able to test Eq
without interference 🐔 🥚
import org.scalacheck.{Arbitrary, Cogen, Gen} | ||
import scala.annotation.tailrec | ||
|
||
object ADTs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So are we dropping testing for sealed traits in favour of enums? I had assumed that we would test both (although that sucks 😂 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not dropping anything 🤔 - we just add more tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh so we are! 🤦
Thank you for all this work 😍 ! New milestone coming out soon? |
We're aiming for at least an RC if not full release ASAP :) |
Unfortunately there is some code duplication because I didn't find a way to make this generic.
But the boilerplate is minimal because we just wrap existing ADTs into a sort of newtype case class.
Closes #425 by making sure it works for all type classes