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

Refcatorings in TASTy reflection interface #5398

Merged
merged 6 commits into from Nov 10, 2018

Conversation

Projects
None yet
3 participants
@nicolasstucki
Copy link
Contributor

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.

@nicolasstucki nicolasstucki force-pushed the dotty-staging:refactor-tasty-reflect branch 8 times, most recently from 83285a2 to e8c6db5 Nov 6, 2018

@nicolasstucki nicolasstucki requested a review from liufengyun Nov 7, 2018

@liufengyun
Copy link
Contributor

liufengyun left a comment

Nice refactoring 👍

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

@allanrenucci

This comment has been minimized.

Copy link
Member

allanrenucci commented Nov 7, 2018

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

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 7, 2018

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

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 7, 2018

Reflection feels like a good name for the reflection API.

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 7, 2018

What would be simpler than ReflectionCore?

@liufengyun

This comment has been minimized.

Copy link
Contributor

liufengyun commented Nov 8, 2018

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

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 8, 2018

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 dotty-staging:refactor-tasty-reflect branch from 11711ed to 0493c53 Nov 8, 2018

@nicolasstucki nicolasstucki force-pushed the dotty-staging:refactor-tasty-reflect branch 2 times, most recently from 689c3fd to 876118e Nov 9, 2018

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 9, 2018

Rebased

nicolasstucki added some commits Nov 7, 2018

Refactor Printer to simplify use
Remove the need to handle path dependent tasty.Reflection

@nicolasstucki nicolasstucki force-pushed the dotty-staging:refactor-tasty-reflect branch from 876118e to 030adb4 Nov 9, 2018

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 9, 2018

Fix conflicts after rebase

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 9, 2018

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

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 9, 2018

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

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 9, 2018

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 lampepfl:master Nov 10, 2018

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

@nicolasstucki nicolasstucki deleted the dotty-staging:refactor-tasty-reflect branch Nov 10, 2018

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