bugfix: Remove additional whitespace from summary #872
Conversation
The recent changes caused the spaces to no longer be removed properly due to the fact that we no longer operate on an actual value when printing one line summary. I tried solving this multiple ways but: - there is no sensible way to just copy any value - using pprint.Tree as itnermediate representation doesn't work as those contain Iterator and are one use only The easiest was to just replace newline with space and then deduplicate whitespace to save space on the online summary. This also makes it consistent across Scala 2 and 3
tgodzik
changed the title
|
@tgodzik I'm struggling to figure out
Do you have a screenshot perhaps of something broken and/or working well? |
When evaluating mutable collections we would always get the last result for everything, because values were evaluated only when printing. I switched to printing them right away to always get the current result.
Printing one line stopped removing additional spaces, so if the printed value had indentation it would all stay in the oneline summary and be quite unreadable.
One line summary should be as compact as possible without losing the visibility, so I decided to just squash any whitespace into a single space.
This broke in Metals scalameta/metals#6523 (and I think it's the only place the one line summary is being used)
|
|
@keynmol what do you think? Or do you see a possibility to improve this approach? |
| out.appendAll(chunk) | ||
|
|
||
| def printOneLine(value: Str, out: StringBuilder, width: Int) = { | ||
| out.appendAll(value.toString().replace("\n", " ").replaceAll("\\s+", " ")).take(width + 1) |
There was a problem hiding this comment.
Hm, I feel like this should also collapse the possible spaces inside a value, e.g. as part of a string constant for example?
But I struggle to reproduce it in tests.
There was a problem hiding this comment.
It might, but we already collapse it even if it contains a newline, so we are already changing the string value. We can't show one line summary otherwise. Maybe we should instead always do ... on newline then and keep spaces in the values?
This is tricky to actually get right since every solution is losing information
There was a problem hiding this comment.
Ok, I don't see a good way of doing it without changing how binding works in general..
That said I'm not deeply familiar with worksheets, will take some time to read it eventually
There was a problem hiding this comment.
We would need a nice intermediate representation of the values, which we can print later on. I almost got it to work with pprint trees, but then pprint uses iterators and I could write things only once. So in the end we would need to reuse pprint trees by either inlining and changing them or by doing a fix upstream. That said, for Scala 2 we don't use pprint, just toString and I am afraid to change it since Martin was mad at us for not doing toString.
All that makes me think that making it work proper currently is not really worth the effort and current approach is good enough 😅
The recent changes caused the spaces to no longer be removed properly due to the fact that we no longer operate on an actual value when printing one line summary.
I tried solving this multiple ways but:
- there is no sensible way to just copy any value
- using pprint.Tree as intermediate representation doesn't work as those contain Iterator and are one use only
The easiest approach was to just replace newline with space and then deduplicate whitespace to save space on the oneline summary. This also makes it consistent across Scala 2 and 3