-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #6270: Allow change of color setting in TASTy Reflect #6276
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
Conversation
4e789cc
to
e1da4a2
Compare
e1da4a2
to
31087c6
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.
Otherwise, LGTM
/** Show a source code like representation of this type */ | ||
/** Show a source code like representation of this type | ||
* Will print Ansi colors if ctx.printColors is enabled. | ||
*/ | ||
def show(implicit ctx: Context): String = | ||
unseal.showCode |
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.
It's better to default to no colors other than ctx.printColors
as that's what users want most of the time.
Otherwise, users will be forced to write x.show(the[Context].withColors)
instead of x.show
.
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 can do either with a default color: Boolean = false
or have two methods:
- show
- showColored
Both are more friendly than x.show(the[Context].withColors)
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 cannot a default the is different for the compiler setting or every other use case will break.
As shown in the example it is easy to add your own version of show
where you disregard the default settings.
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 can always add some variant later
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 it's better to have it now as we go -- we will forget all these minor usability issues. If such usability issues accumulate, the user experience with Tasty Reflect will degrade.
This provides a way to enabled and disable color when printing. The default is set to the current compiler setting.
Now, if someone wants a show that never prints the colors it can be implemented with
Note that
Tree.showCode
will change name toTree.show
to matchExpr.show
.