search

brandonbloom / fipp

Fast Idiomatic Pretty Printer for Clojure
525 stars 44 forks source link

Width consider trailing delimiters #38

Closed mfikes closed 4 years ago

mfikes commented 8 years ago

Consider this data structure which has trailing ] delimiters:

[[[[[(0 1 2 3 4 5 6 7 8 9)]]]]]
1234567890123456789012345678901

(below it is a 1-based column indication for reference).

FIPP will switch to printing this with the numbers aligned vertically at :width threshold of 25 (in other words, at the right-most 9).

An improvement would be to have it instead switch when all the trailing ] characters can no longer fit (say, when width is 30).

A consequence of the current behavior is if the terminal is of width 26 and you are using :width to inform when wrapping should occur, you get output that looks like this:

[[[[[(0 1 2 3 4 5 6 7 8 9)
]]]]]

where the wrapping of the trailing ] characters is essentially being done by the terminal.

If FIPP is working as designed (or perhaps it can't handle this case while preserving its perf guarantees), feel free to close as "won't-fix"; just capturing it here is case it was an oversight.

brandonbloom commented 8 years ago

I'd have to think about this. It's not immediately clear to me what the correct behavior is or why.

brandonbloom commented 8 years ago

Staring at the engine code for a while isn't providing any immediate insights. If you've got the time and energy for deeper analysis, I'd be happy to entertain your thoughts.

mfikes commented 7 years ago

As far as I can tell, Fipp's implementation is faithful to the algorithms described in the literature. If there is any issue, my hunch is that it is a limitation of the underlying algorithm, not a bug in Fipp.

brandonbloom commented 7 years ago

Maybe we could create a primitive for adding extra width to the calculation? So like [:reserve 1] when we render the "["?

mfikes commented 7 years ago

That's an interesting idea, and sounds like it could work.

FWIW, downstream Planck is employing a workaround that appears to work involving trying different widths, but it is a bit messy: https://github.com/mfikes/planck/blob/2.2.0/planck-cljs/src/planck/pprint/width_adjust.cljs

brandonbloom commented 4 years ago

@mfikes Did you ever explore a less messy workaround?

mfikes commented 4 years ago

@brandonbloom No I didn't. I can't recall if I concluded at the time if it was difficult to do while preserving perf.

In any case, this ticket could be closed if there is a desire to not have it just hang around indefinitely.

brandonbloom commented 4 years ago

OK, thanks.