-
Notifications
You must be signed in to change notification settings - Fork 22
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
Implementation of issue #55: Implement type annotations #56
Conversation
f134cde
to
d2146bf
Compare
Thanks a lot and sorry for the delay - I will try to review this soon, but I didn't forget about it |
🙏 |
rebased |
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.
Sorry for taking so long to review 🙈
Overall looks good, the documentation can be improved and some polishing to do.
We should also add some tests with enums - that's the future of ADTs.
modules/deriving/src/main/scala/shapeless3/deriving/annotation.scala
Outdated
Show resolved
Hide resolved
modules/deriving/src/main/scala/shapeless3/deriving/annotation.scala
Outdated
Show resolved
Hide resolved
modules/deriving/src/main/scala/shapeless3/deriving/annotation.scala
Outdated
Show resolved
Hide resolved
modules/deriving/src/main/scala/shapeless3/deriving/annotation.scala
Outdated
Show resolved
Hide resolved
modules/deriving/src/main/scala/shapeless3/deriving/annotation.scala
Outdated
Show resolved
Hide resolved
modules/deriving/src/test/scala/shapeless3/deriving/annotation.scala
Outdated
Show resolved
Hide resolved
modules/deriving/src/test/scala/shapeless3/deriving/annotation.scala
Outdated
Show resolved
Hide resolved
modules/deriving/src/test/scala/shapeless3/deriving/annotation.scala
Outdated
Show resolved
Hide resolved
modules/deriving/src/test/scala/shapeless3/deriving/annotation.scala
Outdated
Show resolved
Hide resolved
modules/deriving/src/test/scala/shapeless3/deriving/annotation.scala
Outdated
Show resolved
Hide resolved
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 addressed my comments - we still need to add more tests but this PR is getting too big.
@joroKr21 thank you for all the fixes and integration! |
@joroKr21 what tests need to be added? |
We were lacking tests with enums - I added one though |
I tried using type annotations on enums, but the following syntax is invalid: enum Control2:
case Automatic @First
case Manual(age: PosInt, email: Email) @Third('!') according to Scala's EBNF, the best I can do is: enum Control2:
case Automatic extends Control2 @First
case Manual(age: PosInt, email: Email) extends Control2 @Third('!') which kills the purpose of the new enum syntax by repeating |
@pgrandjean this is the test I added: @Other @Last(false) enum Control:
@First @Third('!') case Automatic
@Second(100, "@") case Manual(age: PosInt, email: Email) Not sure if that corresponds to your expectations. I think you know better than me how this functionality is supposed to be used. More tests can't hurt, but at least we already have one test for sanity check, so if you want to add more it's up to you. Btw this is not released yet. I got a bit stuck on #36 - wanted to complete it for the next release but it looks still half-baked. I'm thinking of a more useful design so I will leave if for another milestone. |
No description provided.