-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
Overhaul and simplify TPrint implementation #72
Conversation
Also perhaps worth a review by @alexarchambault since this is primarily used in Ammonite |
pprint/src-2/TPrintImpl.scala
Outdated
printSym(x | ||
) ++ printBounds(lo, hi) |
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.
Some weird formatting happening here
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.
fixed this bit of weird formatting
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'm not a macro expert, but nothing seems wrong to me.
Also, this fixes a lot of open bugs, and every test passes.
LGTM! 👍 Thank you!
This reverts commit c66f24d, which was no longer used since 1547da5, and no longer compiles with the metaconfig bump bringing com-lihaoyi/PPrint#72 .
This reverts commit c66f24d, which was no longer used since 1547da5, and no longer compiles with the metaconfig bump bringing com-lihaoyi/PPrint#72 .
Fixes com-lihaoyi/Ammonite#221
Fixes com-lihaoyi/Ammonite#629
Fixes com-lihaoyi/Ammonite#670
Fixes #45
Fixes #44
The old TPrint implementation did a clever thing where it allowed a user to over-ride the TPrinting of a given type by providing an appropriate implicit. While that worked in most cases, it was fiendishly complex, and the intricate nesting of implicit resolution and macro resolution ended up providing and endless source of hard to resolve bugs.
This new implementation is much simpler and less flexible: we simply walk the type data structure in the macro, and spit out a colored
fansi.Str
with the type names hard-coded tofansi.Green
. The only runtime support necessary is in thedef recolor
function, which parses the incomingfansi.Str
and replaces the hardcodedfansi.Green
colors with whatever is specified by the implicitTPrintColors
. As implicits cannot be used to override tprinting anymore, we now have hardcoded support for tprinting functions and tuples.While the old macro generated a complex tree of Scala function calls that is evaluated to generate the output
fansi.Str
at runtime, the new macro simply spits out a singlefansi.Str
that is serialized into ajava.lang.String
and deserialized back into afansi.Str
for usage at runtime. We propagate aWrapType
enumeration up the recursion, to help the callers decide if they need to wrap things in parens or not.This gives up a bit of flexibility, but AFAIK nobody was really using that flexibility anyway. In exchange, we fix a whole bunch of long-standing bugs, and have a drastically simpler implementation.
The fixed bugs are covered by regression unit tests added to
TPrintTests.scala
. All existing tests also pass, so hopefully that'll catch any potential regressions. There's probably more bugs where we're not properly setting or handling theWrapType
, but exhaustively testing/surfacing/fixing all of those is beyond the scope of this PR. For now, I just kept the current set of tests passing.Managed to get the Scala3 side working. I didn't realize how half-baked the Scala3 implementation of TPrint is; so much of the Scala2 functionality just isn't implemented and doesn't work. Nevertheless, fixing that is beyond the scope of this PR. I just kept it green with the existing set of green tests passing (except for the custom tprinter test, which is no longer applicable)
Review by @lolgab.