Add option to display types in worksheets #335
Conversation
There was a problem hiding this comment.
Thank you for this contribution! Just a few comments. I am in general concerned that we will start to introduce many configuration options which will make it difficult to maintain and evolve mdoc's worksheet functionality. A lot of features are easy to implement behind a configuration flag, but it gets really difficult to reason about the behavior when you support multiple configuration options
| | | ||
| |<px()> // "OK": String | ||
| | | ||
| |<{ println("P1"); List(1,2,3) }> // List(1, 2, 3): List[Int] |
There was a problem hiding this comment.
What happens if the value is too large so that it gets truncated?
There was a problem hiding this comment.
It is truncated to exactly match marginWidth (added test to show how it looks)
There was a problem hiding this comment.
I played with BlackWhite.tokenize as it is used for value but it only splits single chunk with full type so not helpful, we could manually cut type earlier if we encounter other char than [a-zA-Z0-9] but I am unsure it would be worth it. Especially for long type name it might make sense to show as much as possible.
There was a problem hiding this comment.
@olafurpg Let me know if you are okay with keeping it as it is for now and improve later or do you have other ideas how to improve it?
tests/unit/src/test/scala/tests/worksheets/WorksheetSuite.scala
Outdated
Show resolved
Hide resolved
|
I would be fine with removing the configuration option and changing the default behavior instead. Unless we have evidence that there is demand for not showing types in the summary, then I think we should be safe to always show types in the summary without an option to configure it. |
| |val b = a | ||
| """.stripMargin, | ||
| """| | ||
| |<val a = Map(List(3) -> Map("v" -> Set(true, false)))> // Map(List(3) -> Map("v... |
There was a problem hiding this comment.
Would it make sense to try and display the type at the beginning so that it doesn't get truncated for large values?
There was a problem hiding this comment.
We could try the following formatting
<val a = Map(List(3) -> Map("v" -> Set(true, false)))> //: Map[List[Int], Set[Boolean]] = Map...
I don't have very strong opinions on what is the best solution here. cc/ @tpolecat any suggestions on how to format types in worksheets when the value is too large and gets truncated to fit the print width?
tgodzik
force-pushed
the
types-in-worksheet
branch
from
4ea6a34 to
e954158
Compare
tgodzik
force-pushed
the
types-in-worksheet
branch
from
e954158 to
1772c9c
Compare
tgodzik
force-pushed
the
types-in-worksheet
branch
from
1772c9c to
f740846
Compare
| @@ -158,7 +158,7 @@ class WorksheetSuite extends BaseSuite { | |||
| |val n = User("Susan") | |||
| |""".stripMargin, | |||
| """|case class User(name: String) | |||
| |<val n = User("Susan")> // User("Susan") | |||
| |<val n = User("Susan")> // : User = User("Susan... | |||
There was a problem hiding this comment.
Would it make sense to special case this scenario and drop the type when the value starts with the same syntax as the type? Feel free to ignore, just an idea
There was a problem hiding this comment.
I think it's better to be consistent, otherwise it might be surprising to users. And it might be quite fragile I think.
It would need to be something like:
if (isSingle && !chunk.startsWith(binder.tpeString + "("))
out
.append(": ")
.append(binder.tpeString)
.append(" = ")
out.append(chunk)There was a problem hiding this comment.
I think we can get back to it if current behavior proves problematic.
One test shows how currently worksheet summary is displayed and second one shows how it is displayed with types enabled.
Made it as an option because I was unsure of how it would look like, but I am more and more convinced it could be just default behaviour.
Also if you have any tricky statements that could be added to test to check how their summary would be rendered - shoot :)