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

Width and height override modifiers #389

Merged
merged 12 commits into from
Oct 17, 2020

Conversation

keynmol
Copy link
Collaborator

@keynmol keynmol commented Sep 18, 2020

Closes #387

My previous implementation had some problems, which are ultimately due to pprint not actually breaking up the input on width - it assumes that the terminal will do it. My ramblings from the previous version are hidden here:

Previous confusion with the way pprint works This is the code to pass width and height overrides to the renderer.

The only problem is that whatever gets rendered by pprint doesn't look right to me. I decided to check it in the ammonite shell:

@ import $ivy.`com.lihaoyi::pprint:0.5.4`
import $ivy.$

@ val x = List.fill(15)("helloasdasdasdasdasdasdasdasd!")
x: List[String] = List(
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasdasdasdasdasd!",
...

@ pprint.PPrinter.BlackWhite.tokenize(x, height=5, width=10, initialOffset=0).foreach(print)
List(
  "helloasdasdasdasdasdasdasda...

@ pprint.PPrinter.BlackWhite.tokenize(x, height=2, width=10, initialOffset=0).foreach(print)
List(
...

@ pprint.PPrinter.BlackWhite.tokenize(x, height=8, width=10, initialOffset=0).foreach(print)
List(
  "helloasdasdasdasdasdasdasdasd!",
  "helloasdasdasdasd...

The numbers don't align, height truncation is incorrect - am I misunderstanding how it's supposed to work?

width=... height=...
image image
Another outdated summary that was before we prevented intra-token line breaks ## width= modifier

image

height= modifier

image

@keynmol
Copy link
Collaborator Author

keynmol commented Sep 29, 2020

@olafurpg I tried to be as cautious as I could with hiding new functionality to avoid breaking existing code, let me know what you think.

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! The change itself looks good, just a few small comments on implementation

case '\n' =>
sb.append("\n// "); lineLength = 0
case ch if lineLength == maxLineLength =>
sb.append(ch); sb.append("\n// "); lineLength = 0
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit skeptical about this line wrapping as it breaks the output at arbitrary offsets. I think there is a risk you line wrap right before a newline like this

List(
  "aaaaa
",
  "bbb"
)

I feel like it would be better to make this line behavior opt-in with an additional modifier mdoc:wrap-width=anywhere where the default value is something like wrap-width=tokens or wrap-width=words. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I can imagine there are cases where users would like to set a smaller width while keeping the current line wrapping behavior (split at tokens/words)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about it, yeah. Width is the problematic one as in the terminal you would expect the behaviour you showed in your example, but in markdown it might look weird.

I need to look closer at what pprint exposes (token-wise) as it indeed would be nicer to break on tokens only, but being able to set anywhere is even better to break long strings (only situation where I can imagine this being useful)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think pprint offers line breaking beyond tokens. I don't mind exposing the ability to break inside tokens but I think it should be an explicit choice from the user

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had a complete brain fart moment :) I struggled to find an example of demonstrating that pprint actually does wrap at given width, around tokens.


```scala mdoc:width=50
List.fill(10)(List(1,2,3,4,5,6,7,8,9,10))
```

```scala
List.fill(10)(List(1,2,3,4,5,6,7,8,9,10))
// res0: List[List[Int]] = List(
//   List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10),
//   List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10),
//   List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10),
//   List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10),
//   List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10),
//   List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10),
//   List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10),
//   List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10),
//   List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10),
//   List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10)
// )
```




```scala mdoc:width=20
List.fill(10)(List(1,2,3,4,5,6,7,8,9,10))
```

```scala
List.fill(10)(List(1,2,3,4,5,6,7,8,9,10))
// res1: List[List[Int]] = List(
//   List(
//     1,
//     2,
//     3,
//     4,
//     5,
//     6,
//     7,
//     8,
//     9,
//     10
//   ),
//   List(
//     1,
//     2,
//     3,
//     4,
//     5,
//     6,
//     7,
//     8,
//     9,
//     10
//   ),
//   List(
//     1,
//     2,
//     3,
//     4,
//     5,
//     6,
//     7,
//     8,
//     9,
//     10
//   ),
//   List(
//     1,
//     2,
//     3,
//     4,
//     5,
//     6,
//     7,
//     8,
//     9,
//     10
//   ),
// ...
```

which shows that it works (and I don't need to do manual breaking).

For my usage I don't actually need this "break anywhere" behaviour because it's unpredictable and can lead to bad rendering.

So unless you explicitly want to keep it in (guarded by a custom modifier), I will remove this function altogether and modify the tests - how does that sound?

@keynmol keynmol requested a review from olafurpg September 30, 2020 16:54
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

I'm sorry for the delayed review. I only have two nitpick comments, otherwise this looks ready for merge!

def unapply(string: String): Option[Mod] = {
all.find(_.toString.equalsIgnoreCase(string))
static.find(_.toString.equalsIgnoreCase(string)) orElse parametric
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's avoid non-symbolic infix operators

Suggested change
static.find(_.toString.equalsIgnoreCase(string)) orElse parametric
static.find(_.toString.equalsIgnoreCase(string)).orElse {
parametric.flatMap(check => check(string)).headOption
}

@@ -38,7 +48,16 @@ object Mod {
ToString,
Nest
)

def parametric: List[String => Option[Mod]] =
Copy link
Member

Choose a reason for hiding this comment

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

nit: I feel like the following solution is a bit nicer

import scala.util.Try
sealed abstract class Mod extends Product with Serializable
case object Passthrough extends Mod
val static = List[Mod](Passthrough)
case class Width(n: Int) extends Mod
case class Heigth(n: Int) extends Mod
val ToWidth = "width=(\\d+)".r
val ToHeight = "height=(\\d+)".r
object ToInt {
  def unapply(s: String): Option[Int] = Try(s.toInt).toOption
}

List("width=24", "height=42", "width=abc").map {
  case ToWidth(ToInt(n)) => Some(Width(n))
  case ToHeight(ToInt(n)) => Some(Heigth(n))
  case _ => None
} // List(Some(Width(24)), Some(Height(42)), None)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice.

I've changed it very slightly, how does 21106b3 look?

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you! I apologize the slow review 😅

@olafurpg olafurpg merged commit c5342f5 into scalameta:master Oct 17, 2020
@olafurpg
Copy link
Member

Release this as 2.2.10!

@keynmol keynmol deleted the width-height-modifiers branch October 17, 2020 18:53
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.

--screen-height parameter is not propagated to ReplVariablePrinter
2 participants