-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add zero-width text nodes and styled nodes. #184
Conversation
This change introduces a new concept: ZeroWidth(_). This is basically a Text(s) node whose internal string doesn't count against the "budget" that Paiges uses to render. The expectation is that this will be used to support "invisible" pieces of text, such as ANSI control characters. We do expose a zeroWidth() constructor, but this is a power user feature and it's definitely possible to use this incorrectly. We also added a Styled(_, _) node, which applies a style to a Doc. During rendering this will translate into zero-width nodes, but unlike zero-width nodes, styles are composable. Currently styles are provided for ANSI and XTerm control sequences.
Codecov Report
@@ Coverage Diff @@
## master #184 +/- ##
=========================================
+ Coverage 98.12% 98.3% +0.18%
=========================================
Files 6 7 +1
Lines 320 414 +94
Branches 38 32 -6
=========================================
+ Hits 314 407 +93
- Misses 6 7 +1
Continue to review full report at Codecov.
|
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 is super exciting!
If you have time check the coverage. It seems we aren’t covering Style well.
@@ -180,6 +190,7 @@ sealed abstract class Doc extends Product with Serializable { | |||
case d1 :: tail => loop(d1, tail) | |||
case Nil => true | |||
} | |||
case Styled(_, _) => false |
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 this is wrong now after we removed isEmpty check in style. I think we can recurse on d.
@@ -192,6 +203,9 @@ sealed abstract class Doc extends Product with Serializable { | |||
case Text(s) => | |||
// shouldn't be empty by construction, but defensive | |||
s.isEmpty && loop(Empty, stack) | |||
case ZeroWidth(s) => | |||
// shouldn't be empty by construction, but defensive |
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 don’t think that comment is true anymore.
def color(r: Double, g: Double, b: Double): Style = { | ||
// require(0.0 <= r && r < 1.0, s"red: $r") | ||
// require(0.0 <= g && g < 1.0, s"red: $r") | ||
// require(0.0 <= b && b < 1.0, s"red: $r") |
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.
Maybe uncomment these?
|
||
object Fg { | ||
|
||
def color(r: Double, g: Double, b: Double): Style = { |
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 am nervous about two color methods both taking three numbers with different range (0 to 5 vs 0.0 to 1.0). Maybe colorFrac or colorDouble or something?
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.
Also we only have this on Fg not on the Bg.
Black, Red, Green, Yellow, Blue, Magenta, Cyan, White, Default, | ||
BrightBlack, BrightRed, BrightGreen, BrightYellow, BrightBlue, | ||
BrightMagenta, BrightCyan, BrightWhite) | ||
} |
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.
Add XTerm generators as well?
@@ -178,14 +178,16 @@ class PaigesScalacheckTest extends OurFunSuite { | |||
val maxR = d.render(m) | |||
val justAfter = (1 to 20).iterator | |||
val goodW = (justAfter ++ ws.iterator).map { w => (m + w) max m } | |||
assert(goodW.forall { w => d.render(w) == maxR }) | |||
goodW.foreach { w => | |||
assert(d.render(w) == maxR, repr(d)) |
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.
Does assertDoc work here?
I’d love feedback from @seanmcl and @DavidGregory084 since they opened the original issues. My feeling is this seems to cover everything you can do with: https://hackage.haskell.org/package/ansi-wl-pprint-0.6.8.1/docs/Text-PrettyPrint-ANSI-Leijen.html so I hope it will meet our needs for the short term. |
|
Yeah, I need to step away for a bit but I think getting all the style stuff covered and consistent shouldn't be too hard. |
Wow thanks guys for all your work on this! I won't be able to get back to the project I was working on until next week but I'm excited to try this out! 👍 💯 |
This removes some rendering complexity. We support the same public API by implementing the .style() method with ZeroWidth nodes directly.
This change introduces a new concept:
ZeroWidth(_)
. This is basicallya Text(s) node whose internal string doesn't count against the
"budget" that Paiges uses to render. The expectation is that this will
be used to support "invisible" pieces of text, such as ANSI control
characters. We do expose a
zeroWidth()
constructor, but this is apower user feature and it's definitely possible to use this
incorrectly.
We also added a
Styled(_, _)
node, which applies a style to aDoc
.During rendering this will translate into zero-width nodes, but unlike
zero-width nodes, styles are composable. Currently styles are provided
for ANSI and XTerm control sequences.