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 pretty-printing of annotations on symbols #148

Merged
merged 2 commits into from
Apr 15, 2021

Conversation

Strum355
Copy link
Member

@Strum355 Strum355 commented Apr 8, 2021

Previously, symbols that had annotations did not render those in the formatter/pretty-printer.

This PR introduces 2 new semanticdb spec changes that need to be upstreamed:

  1. A new AnnotationTree type as part of the Tree collection.
    • This replaces the Annotation type used in SymbolInformation (and AnnotatedType) while having the same index 1 field and an additional repeated Tree field for parameters
  2. A new AssignTree type as part of the Tree collection, for assignment expressions.

In combination, these add support to semanticdb for modeling the full JLS annotation spec, which we utilize to pretty-print.


Follow-up PRs:

  1. Add helper functions for building many of the semanticdb constructs add pretty-printing of annotations on symbols #148 (comment) ✔️
  2. Move the semanticdbXXX methods in SemanticdbVisitor to a separate class add pretty-printing of annotations on symbols #148 (comment) ✔️

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Phenomenal work getting annotations working! SemanticDB has never properly supported annotations because they're too complicated to model in Scala. It's a fantastic breakthrough that we can properly support Java annotations.

A few comments, the most important ones relate to changes in the SemanticDB spec.

@Strum355 Strum355 requested a review from olafurpg April 9, 2021 19:17
@Strum355
Copy link
Member Author

Strum355 commented Apr 9, 2021

Ive updated the PR description with the changes that arose from the review 🙂 @olafurpg

@Strum355 Strum355 force-pushed the nsc/symbol-annotation-format branch from a8e5910 to e5f14a2 Compare April 9, 2021 20:24
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Outstanding work 👏 This is ready to merge!

It would be good to followup soon with the upstream spec changes.

@Strum355 Strum355 force-pushed the nsc/symbol-annotation-format branch 2 times, most recently from e419be0 to 2d36a96 Compare April 12, 2021 22:17
@Strum355
Copy link
Member Author

Merging ahead of scalameta/scalameta#2281. We dont anticipate the PR to result in any byte-level breaking changes to what is being merged here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants