-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Rework TastyInspector API to allow inspection of all files #10792
Rework TastyInspector API to allow inspection of all files #10792
Conversation
This is the API that everyone has been asking for. Now we can inspect all TASTy files sources at once. @abgruszecki could you have a look at the |
7524970
to
780d890
Compare
5e32f93
to
8baf71e
Compare
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.
Overall LGTM, I have only one small comment.
8baf71e
to
854d5ef
Compare
@abgruszecki I added a |
It looks good. Having the Quotes instance inside Tasty shouldn't hurt. |
a7e2628
to
5575ea2
Compare
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 we could use a test that implements Inspector#inspect
, uses the outer Quotes
to retrieve a symbol, and compares it with the symbol of the nested ast - mostly as a sanity check that the types work out. Otherwise, LGTM.
Could you give me an outline of the test you have in mind? |
5575ea2
to
e190f91
Compare
@abgruszecki after our discussion about the current tests is it ok to merge this PR? |
@nicolasstucki yep, no issue on that end. |
e190f91
to
9ec2464
Compare
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.
LGTM
|
||
/** Called after all compilation units are processed */ | ||
protected def postProcess(using Quotes): Unit = () | ||
object TastyInspector: |
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.
Minor: what about just call it Tasty
? The rationale is to avoid the conceptual link with Inspector
.
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 already have trait called Tasty
that means something different. Maybe we need another name.
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.
Then it's fine to keep the current name.
9ec2464
to
795e465
Compare
795e465
to
e903941
Compare
An alternative to #10777
Change the API to allow inspection of all trees at the same time.
Porting scala3doc is left for a future PR.