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

[Inspector] Configure truncation limits #111

Merged
merged 1 commit into from Apr 11, 2021

Conversation

pdbrown
Copy link
Contributor

@pdbrown pdbrown commented Apr 9, 2021

The limits after which the inspector truncates collection members are now
configurable. Previously they were hardcoded to 5 and 150 for collection and
atom (non-collection) members.


Hi there!

I've been using cider-inspect to great effect recently. Occasionally, I run into cases where it would be really nice to see a collection like:

[[1 2 3 4 5 :some]
 [1 2 3 4 5 :important]
 [1 2 3 4 5 :thing]]

but it currently appears as:

Class: clojure.lang.PersistentVector
Contents: 
0. [ 1 2 3 4 5 ... ]
1. [ 1 2 3 4 5 ... ]
2. [ 1 2 3 4 5 ... ]

and I have to nav into each element one by one.

This change, and 2 more like it in cider-nrepl and cider, make the nested collection size limit configurable. While I was at it, I figured I might as well make the non-collection "atom" member sizes configurable too.


  • 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)

The limits after which the inspector truncates collection members are now
configurable. Previously they were hardcoded to 5 and 150 for collection and
atom (non-collection) members.
(defn- short? [coll]
(<= (count coll) 5))
(def ^:private ^:dynamic *max-atom-length* 150)
(def ^:private ^:dynamic *max-coll-size* 5)
Copy link
Member

Choose a reason for hiding this comment

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

Why is one size and the other length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah I think these names are a sign of my java heritage, and am happy to adjust them if needed. I started with max-string-length and max-coll-count, but "count" sounded weird to me because it's more about "how much of this collection do I want to see" vs what could be taken as "how many collections do I want to see". It turned out "string" was too specific, so it became max-atom-length, and java collections have sizes, so I ended up with max-coll-size.
Later on, I realized it's really more like max-branching-factor, and that it would be cool, if instead of having to set it manually, I could just tell inspector how much data I want to see, and have it go figure reasonable depths and breadths on a per-node basis. That'd be for another time though!

Copy link
Member

Choose a reason for hiding this comment

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

Naming is hard. :-) Thanks for explaining your reasoning. It makes sense to me.

@bbatsov
Copy link
Member

bbatsov commented Apr 10, 2021

While I was at it, I figured I might as well make the non-collection "atom" member sizes configurable too.

Great idea. Overall the change seems solid to me. For a while it had bothered me that those were hardcoded and it's nice that we're finally fixing this.

@yuhan0
Copy link
Contributor

yuhan0 commented Apr 10, 2021

I wonder if it's better to make the dynamic vars public instead- that way Orchard doesn't have to expose a setter API, and cider-nrepl can set! the vars directly or bind them on a per-call basis.

@bbatsov
Copy link
Member

bbatsov commented Apr 10, 2021

I thought about this myself, but I see @pdbrown just followed the existing patterns in the code. The inspector has a very messy history as it's a direct descendant of the oldest code from swank-clojure and the codebase would probably look very differently if we wrote it from scratch today, but it is how it is.

I'm fine with using dyn vars for configuration, but I like to give people some cleaner alternative as well. E.g. in nREPL's print middleware you can either set dyn vars or override them with request params https://github.com/nrepl/nrepl/blob/master/src/clojure/nrepl/middleware/print.clj#L169

@pdbrown
Copy link
Contributor Author

pdbrown commented Apr 10, 2021

Yeah that's basically it. I guess I thought it was nice to keep the inspector's configuration together on the "inspector state" and then keep the dynamic scope as small as possible, so then we can do this: https://github.com/clojure-emacs/cider-nrepl/pull/694/files#diff-2d94dd0e5df3764b260b1bf0208d491c564b44a95f47aa0678d7046c7fd0d754R28
FWIW, that ^:private hint is hardly enforced, and could be lifted later if we wanted to. The tests already bind these.

@bbatsov bbatsov merged commit f763175 into clojure-emacs:master Apr 11, 2021
@bbatsov
Copy link
Member

bbatsov commented Apr 11, 2021

Thanks! I'll cut a new release soon.

@yuhan0
Copy link
Contributor

yuhan0 commented Apr 11, 2021

I just noticed that inspect-value for :map-long wasn't changed to show *max-coll-size* number of keys in the same way as other collections, was this an oversight?

@pdbrown
Copy link
Contributor Author

pdbrown commented Apr 12, 2021

Ah yeah, I noticed :map-long but decided to leave it as is to keep this change limited to the configuration feature and leave behavior unaffected. Seems worth fixing like you do in #115 though!

@yuhan0
Copy link
Contributor

yuhan0 commented Apr 12, 2021

Yeah, I always thought it was a bug to have maps suddenly display fewer items the more you add to them.

0.  { :a 1, :b 2, :c 3 }
1.  { :a 1, :b 2, :c 3, :d 4 }
2.  { :a 1, :b 2, :c 3, :d 4, :e 5 }
3.  { :a 1, ... }
4.  { :a 1, ... }

And it's usually not the one key/val I'm interested in either - I have another experiment in prioritizing/ordering keys but that's for another time.

@bbatsov unless you want to group related inspector changes together for a release? I can put up a draft PR of that.

@pdbrown pdbrown deleted the feature/display-length branch April 13, 2021 03:18
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.

None yet

3 participants