[inspect] Extract inspect-value into a separate printer implementation

[alexander-yakushev]

Before submitting a PR make sure the following things have been done:

• The commits are consistent with our contribution guidelines
• You've added tests to cover your change(s)
• All tests are passing
• The new code is not generating reflection warnings
• You've updated the changelog (if adding/changing user-visible functionality)

Thanks!

[vemv]

Seems like a good batch of refinements!

Probably the best 'review' I can provide is suggesting that you make install Orchard and then cider-nrepl for a few days - good way to catch anything that might slip by

[alexander-yakushev]

Change of plans.

orchard.inspect/inspect-value is basically a custom value-printing implementation (akin to print-method). It deserves its own place to live, and I have a long-overdue wish to use it in the CIDER debugger as well.

The extracted printer contains the previous performance improvements and new truncation logic. Now it supports:

• *print-length* (instead of *max-coll-size*)
• *print-level* – to limit printing of deeply nested collections
• *max-atom-length* – it was there before. Now it truncates inputs at the Writer level, so it will automatically apply to external print-method implementations.
• *max-total-length* – new limit, allows to hard-limit the maximum size of the printed string. The string is truncated at the Writer level, but a cooperating printer implementation will also stop evaluating and printing values once the limit is reached.

@vemv The inspector has a pretty good test coverage, so I'm quite optimistic about the changes. Something might change visually but not structurally. Anyway, I would appreciate even a brief review.

[vemv]

Seems something nice to unit-test in isolation

[alexander-yakushev]

Somewhat agree, but I also believe that test/orchard/print_test.clj unit tests cover this quite well.

[vemv]

That would seem more of a side coverage, in a unit test maintainers can have isolated examples and explicitly going over the edge cases

(and pigging back on the topic of today's another PR, not everyone wants to read Java all the time - worth optimizing for that)

[alexander-yakushev]

Done 👍

[alexander-yakushev]

Extra questions to consider:

• What should be the default value for *max-total-length*? I've put 2000 as an example, it looks like this (arrows highlight the truncation points)

- What should be the default value for `*print-level*`?

[vemv]

I'd say that by default, on an average monitor we should strive to show one line of text at most - or maybe a handful, IDK.

The reason being, such large k-v entries can easily bury other k-v entries.

User can always drill deeper into the data if they need more detail.

(For clarity - before this PR the behavior doesn't seem ideal to me at all)

Perhaps we can leave this aspect essentially unaltered in this PR? I'm not a fan of big PRs anyway.

[alexander-yakushev]

Perhaps we can leave this aspect essentially unaltered in this PR? I'm not a fan of big PRs anyway.

Do you mean disabling long printed value truncation for now?

[vemv]

I'd simply try to keep behavior as close to the current one as possible.

But we can certainly try to refine it afterwards.

[alexander-yakushev]

FYI, the current behavior is this:).

But if you want to do this in two-step, first merge all the necessary foundation and second change the user experience, then I'm fine with that.

[vemv]

I've never gotten a SO in practice.

[alexander-yakushev]

It happens sometimes when you work with Java object hierarchies.

[alexander-yakushev]

Regarding why I want this for the debugger:

Around 40% of the time when I use CIDER debugger, I need to perform an extra dance to avoid the step debugger landing on a piece of data that is big (say, ~50-100Mb) because it tries rendering it which hurts both Clojure and Emacs side of the debugger. By unifying the printers that Inspector and Debugger use, and by constraining how much work they do when displaying values, we can greatly improve the UX.

[alexander-yakushev]

Here's my proposal to take it piecemeal. First step:

• Add orchard.print that supports all four truncation customizations (current max-coll-size and max-atom-length, and new max-value-length and max-nested-depth).
• For the inspector, the default max-nested-depth remains nil (unlimited) for now.
• For the inspector, the default max-value-length becomes 50000. This corresponds to 10 full screens of text on my display. I believe this change will not impact UX anywhere except in degenerate cases where printing the full value was not certainly not desired.

Later, we can think whether changing these defaults is necessary and what are the optimal values.

[bbatsov]

Around 40% of the time when I use CIDER debugger, I need to perform an extra dance to avoid the step debugger landing on a piece of data that is big (say, ~50-100Mb) because it tries rendering it which hurts both Clojure and Emacs side of the debugger. By unifying the printers that Inspector and Debugger use, and by constraining how much work they do when displaying values, we can greatly improve the UX.

I agree.

@alexander-yakushev I'm fine with your piecemeal proposal, although I'd also be OK with making all changes at once. I really doubt this will affect someone negatively.

[bbatsov]

Don't forget to add some :added metadata for this ns.

[vemv]

We have existing patterns in this ns such as (update-in [:rendered] concat (list expr)) so I'd rather not have this touched.

[alexander-yakushev]

I changed that as well. Concat is generally an antipattern. https://stuartsierra.com/2015/04/26/clojure-donts-concat

[vemv]

Alright

Either way, any change that has the risk of going wrong deserves its own commit, if not its own PR.

PRs which aren't constrained to a single topic are harder to review.

Please don't make maintainers squint/head-scratch at changes - present them nicely so that it's all safe and streamlined.

Hope it helps for future occasions!

[alexander-yakushev]

Removed that commit from this PR.

[vemv]

Great work - thanks!