-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Rename QuoteContext to Quotes #10432
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
Rename QuoteContext to Quotes #10432
Conversation
9c973ed to
fe6844e
Compare
liufengyun
left a comment
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
using Quotes reads well, shorter, (almost) as clear in meaning as QuoteContext.
withQuotes is not as clear as withQuoteContext, but it only exists in the staging lib and not used in macros.
|
|
||
| ```scala | ||
| given derived[T: Type](using qctx: QuoteContext) as Expr[Eq[T]] | ||
| given derived[T: Type](using qctx: Quotes) as Expr[Eq[T]] |
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.
What is the recommended name to replace qctx? qt or qs?
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.
The recommendation is not to name it. The question is what should we rename def qctx(using...)... to? I would like to try q as it would make it short for q.reflect.Tree.
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.
Or we could call it quotes to have a meaningful 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.
This PR is quite large as it is. It might be better to rename that one in another PR
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'd actually be in favour of renaming qctx to q, and also for creating qr as an alias to q.reflect.
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 q is acceptable, given that common usage will make it clear. However, I find qr is too obscure.
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.
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.
@liufengyun qr will stop being obscure if it will be the standard way to refer to the .reflect object. Based on my experience from the doctool codebase, I can tell you that qctx.reflect feels very verbose. Any codebase that uses Tasty Inspector seriously will run into the same sort of pain.
Anyway, given that we are migrating to an even more verbose name for qctx, I think we will define an alias for quotes.reflect in the doctool.
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.
IMHO introducing two magic names like q and qr will be too much for the learner.
I think defining a shorter name like qr for q.reflect in the user library is reasonable.
4869939 to
a5046b9
Compare
a5046b9 to
90a3a79
Compare
|
@liufengyun the second commit renamed everything in the community build and in the compiler. This will need a review too. |
90a3a79 to
66c1463
Compare
b5f183e to
5ef454c
Compare
liufengyun
left a comment
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
`QuoteContext` is required on all signatures. To make it simpler to keep signatures on one line we shorten the name to `Quotes`. This name will usually be used in `(using Quotes)` which reads as _using quotes_ and descibes exactly why this contextual parameter is the. ```diff - def f[T: Type](x: Expr[T])(using QuoteContext): Expr[T] = ... + def f[T: Type](x: Expr[T])(using Quotes): Expr[T] = ... ```
54c6c34 to
80ed86a
Compare
QuoteContextis required on all signatures. To make it simpler to keep signatures on one line we shorten the name toQuotes. This name will usually be used in(using Quotes)which reads as using quotes and describes exactly why this contextual parameter is there for.