Skip to content

Refcatorings in TASTy reflection interface #5398

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

Merged
merged 6 commits into from
Nov 10, 2018

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Nov 6, 2018

This PR focuse on refactorings that have been on hold to avoid rebasing other PRs. Now is a good moment to do it as other PRs won't be too hard to rebase on this.

  • Rename the reflection API from tasty.Tasty to tasty.Reflection
  • Rename toTasty/toExpr (quoted to tasty trees / tasty trees to quoted) to relfect/reify in the tasty.Reflection interface
  • Rename Show to Printer
  • Add showCode to tasty trees
  • Refactor util.{TreeAcumulator, TreeTraverser} to not need and explicit singleton type parameter ([T <: Tasty with Singleton](tasty: T)). Move them inside tasty.Reflection.

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

Nice refactoring 👍

If we can rename ReflectionCore to something simpler, that would be better.

@allanrenucci
Copy link
Contributor

Can we discuss these renames during the next meeting?

It would be nice to reach a consensus and then try to avoid breaking the reflect API in the future.

I am personally not convinced by the following:

  • toTasty/toExpr -> reflect/reify
  • Tasty -> Reflection

@nicolasstucki
Copy link
Contributor Author

toTasty is not meaningful and toExpr is overloaded with the lifters.

@nicolasstucki
Copy link
Contributor Author

Reflection feels like a good name for the reflection API.

@nicolasstucki
Copy link
Contributor Author

What would be simpler than ReflectionCore?

@liufengyun
Copy link
Contributor

@nicolasstucki I was thinking something like Types or just Core. But as this is just an internal name, it does not matter much.

@nicolasstucki
Copy link
Contributor Author

Types would get confused with the part that has the extractors for Types. Core is good, I will shorten it.

@nicolasstucki nicolasstucki force-pushed the refactor-tasty-reflect branch from 11711ed to 0493c53 Compare November 8, 2018 13:17
@nicolasstucki nicolasstucki force-pushed the refactor-tasty-reflect branch 2 times, most recently from 689c3fd to 876118e Compare November 9, 2018 11:22
@nicolasstucki
Copy link
Contributor Author

Rebased

Remove the need to handle path dependent tasty.Reflection
@nicolasstucki nicolasstucki force-pushed the refactor-tasty-reflect branch from 876118e to 030adb4 Compare November 9, 2018 11:36
@nicolasstucki
Copy link
Contributor Author

Fix conflicts after rebase

@nicolasstucki
Copy link
Contributor Author

To simplify rebasing, we should merge this one before #5416, #5388 and any new PR using TASTy reflect.

@nicolasstucki
Copy link
Contributor Author

I should merge this before the meeting to get unblocked. WDYT @liufengyun. If needed we can rename methods later.

@nicolasstucki
Copy link
Contributor Author

FYI people seem to be familiar with the reflect/reify names because of all the keynotes @odersky presented with this notation.

@nicolasstucki nicolasstucki merged commit b0a20fc into scala:master Nov 10, 2018
@nicolasstucki nicolasstucki deleted the refactor-tasty-reflect branch November 10, 2018 21:44
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.

3 participants