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

Rework reflect show API #10661

Merged
merged 4 commits into from Dec 8, 2020
Merged

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki force-pushed the rework-reflect-show-api branch 3 times, most recently from a2cab66 to c40a99e Compare December 6, 2020 10:52
@nicolasstucki nicolasstucki force-pushed the rework-reflect-show-api branch 3 times, most recently from 643642f to da9861a Compare December 7, 2020 07:53
@nicolasstucki nicolasstucki marked this pull request as ready for review December 7, 2020 09:08
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.

Otherwise, LGTM

library/src/scala/quoted/Quotes.scala Outdated Show resolved Hide resolved
def ConstantAnsiCode: Printer[Constant]

/** Prints a pattern like representation of the `Constant`. */
def ConstantCases: Printer[Constant]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are Constant printers necessary? I assume they are part of the tree printers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Constant is not a Tree. But indeed, the TreeCases printer does print Constant using the equivalent of ConstantCases.

It is necessary if you have a Constant and want to show it without having to put it back in a Literal to then call show.

Copy link
Contributor

Choose a reason for hiding this comment

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

If they are not very useful, then I'd suggest removing them to make it easier for programmers to choose printers they care about most of the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified the Constant.show to not take a printer

library/src/scala/quoted/Quotes.scala Outdated Show resolved Hide resolved
library/src/scala/quoted/Quotes.scala Outdated Show resolved Hide resolved
* Add `reflect.Printer[T]` type class
* Define `show` method in reflect based on `Printer`
* Provide a single default `show` for `Expr` and `Type` (use `reflect` to access other printers)
* Only define printers that make sence for the given types.
* Remove `Symbol.show`. Use directly `Symbol.fullName` instead.
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.

LGTM

@liufengyun liufengyun merged commit 926dbc0 into scala:master Dec 8, 2020
@liufengyun liufengyun deleted the rework-reflect-show-api branch December 8, 2020 13:43
/** Type class used in `show` methods to provide customizable `String` representations */
trait Printer[T]:
/** Show the arguments as a `String` */
def show(x: T): String
Copy link
Member

Choose a reason for hiding this comment

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

With Printer being a member of the Reflection trait, it seems very tricky for users to write their own custom printer that works with any Reflection context. Is there an example of how to do that? If it's not possible maybe we can come up with a better API here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are examples of how to use the Tree map that is quite similar. I will add an example for printer.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be nice.

@deusaquilus
Copy link
Contributor

deusaquilus commented Dec 15, 2020

So instead of:

term.showExtractors

We do:

Printer.TreeStructure.show(term)

That's quite a bit more that user a needs to remember.

@nicolasstucki
Copy link
Contributor Author

The idea is to use term.show(using Printer.TreeStructure). If you are using an IDE, it should help you autocomplete after the Printer. which means you don't need to really remember the name by heart.

@deusaquilus
Copy link
Contributor

deusaquilus commented Dec 15, 2020

Thanks for the info. The thing about showExtractors is that it's really, really useful as a one-off kind of call where I'm in some spot and want to quickly get a tree from there and never need it again (it also will be very useful inside debuggers when they happen). It's hard to get anything so quick if you need to think about what implicits to inject...

I can think of some alternatives would do codebase-wide but this call was so, so nice to quickly use!

@deusaquilus
Copy link
Contributor

I remember there was even a compiler error at some point where if you didn't match a tree correctly it would tell you to use .showExtractors to get the correct one. That was really, nice! These kinds of little considerations go a really long way to encourage new users.

@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants