Skip to content
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 support to extract all variable or type annotations (issue #1051) #1058

Merged
merged 5 commits into from
Apr 27, 2021
Merged

Add support to extract all variable or type annotations (issue #1051) #1058

merged 5 commits into from
Apr 27, 2021

Conversation

pgrandjean
Copy link
Contributor

@pgrandjean pgrandjean commented Nov 18, 2020

Following issue #1051, adding support to extract all variable or type annotations from case classes.

Closes #1051

Base automatically changed from master to main March 19, 2021 12:59
@joroKr21
Copy link
Collaborator

That looks binary compatible - it could make it into a 2.3 release

Copy link
Owner

@milessabin milessabin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Collaborator

@joroKr21 joroKr21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pgrandjean thank you, the implementation looks great 👍

I am only missing tests for all annotations of sealed traits

core/src/test/scala/shapeless/annotation.scala Outdated Show resolved Hide resolved
core/src/test/scala/shapeless/annotation.scala Outdated Show resolved Hide resolved
@pgrandjean
Copy link
Contributor Author

@pgrandjean thank you, the implementation looks great 👍

I am only missing tests for all annotations of sealed traits

Also added tests for sealed trait, although there are no type annotations for sealed traits.

@pgrandjean
Copy link
Contributor Author

branch issue_1051 rebased on main.

@joroKr21
Copy link
Collaborator

Also added tests for sealed trait, although there are no type annotations for sealed traits.

Hmm it's technically possible:

case class Ann(s: String) extends StaticAnnotation
sealed trait Foo
case object Bar extends Foo @Ann("as")

But the current implementation doesn't find them and I wonder if it makes any sense.
But I also don't know what's the use case of type annotations in general.

@joroKr21
Copy link
Collaborator

joroKr21 commented Apr 26, 2021

I'm trying to pull that thread but I encountered an annoying issue - dealias behaves differently in the compiler (preserving type annotations) and in macros (discarding type annotations) 😕

@pgrandjean
Copy link
Contributor Author

pgrandjean commented Apr 26, 2021

Hmm it's technically possible:

You're right. I don't know why I was convinced of the opposite. Sorry for that.

AllTypeAnnotations fixed for sealed traits. In this case, the type annotations are attached to the parent class (Base2), so one needs to go one layer deeper in the AST in function extract.

Copy link
Collaborator

@joroKr21 joroKr21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pgrandjean that looks good to me already. Sorry to keep expanding the scope but I'm wondering if we should make something like this work, which seems to me like a more compelling use case for type annotations:

  final case class validated(id: String) extends StaticAnnotation
  type PosInt = Int @validated("positive") 
  type Email = String @validated("email")
  final case class User(email: Email, age: PosInt)

I tried it but it seems that .dealias sometimes preserves annotations and sometimes loses them. Anyway I'm happy to merge this as is, just suggesting what we could do next.

@pgrandjean
Copy link
Contributor Author

pgrandjean commented Apr 27, 2021

@joroKr21 I think it is possible to achieve this without dealias. Just pushed a change to the extract function to take into account TypeRefs. Test added, seems to work.

Copy link
Collaborator

@joroKr21 joroKr21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing 🙏

core/src/main/scala/shapeless/annotation.scala Outdated Show resolved Hide resolved
@joroKr21
Copy link
Collaborator

Thank you @pgrandjean for your work and patience and congratulations on your first merge to shapeless! 🥳

@joroKr21 joroKr21 merged commit 4693565 into milessabin:main Apr 27, 2021
@pgrandjean
Copy link
Contributor Author

Thank you @joroKr21 !

@joroKr21
Copy link
Collaborator

@pgrandjean wow I completely forgot about #925, sorry!

GitHub lied to me and told me you're a first time contributor - it even asked me to approve your CI run 🙈
But having a bad memory is my own fault 😄 so I guess congratulations on your second PR 👍

I will try to backport them to the 2.3 branch soon, I think there will be no issues 🤞

@pgrandjean pgrandjean deleted the issue_1051 branch April 28, 2021 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for type parameters in annotations
3 participants