clojure-emacs / orchard

A fertile ground for Clojure tooling
Eclipse Public License 1.0
326 stars 54 forks source link

[inspector] Unify inspector entrypoints #244

Closed alexander-yakushev closed 5 months ago

alexander-yakushev commented 5 months ago

I tried to make sense of all the ways to initiate a new inspection or re-inspection – start, fresh, clear. The problem with current state is that "an empty inspector" is not really a thing, it has to be inspecting something. So, an API call like (fresh) doesn't really make sense. Also, reusing an inspector to inspect a new object (unrelated to the current one) actually means only reusing the dynamically changed config (page size, truncation limits, etc.); we don't want to reuse any part of the state.

The updated API has start as the single entrypoint into an inspector. 1-arg takes a value to be inspected and returns a rendered inspector object. 2-arg takes a value and the config. An already existing inspector object can be passed as the config. There are no other legal ways to obtain a new inspector object besides start.

fresh and clear functions were retained and marked as deprecated in case somebody uses them.

bbatsov commented 5 months ago

I agree that the current API is weird. That's mostly related to the fact that some parts of it are as old as swank-clojure from where this code was originally imported:

Originally it was part of swank-clojure, afterwards it was moved to javert, then forked to another project from which it was contributed to cider-nrepl. Finally cider-nrepl was split into two libraries and the code ended up here.

It's great you're pushing forward some efforts to clean up the codebase.

fresh and clear functions were retained and marked as deprecated in case somebody uses them.

I'm around 99% sure cider-nrepl is the only user of this part of Orchard, but that's a prudent move regardless.

bbatsov commented 5 months ago

One more thought about this PR - perhaps it'd be even better to add a function called inspect (start can be it's alias), as I think this reflects better how the inspector works - you said yourself it needs a value to inspect.

alexander-yakushev commented 5 months ago

This makes sense, but we already have inspect in the namespace (public no less) which is an intermediate multimethod. Renaming all those methods to something else is possible but it's easier to leave this as is.

bbatsov commented 5 months ago

This makes sense, but we already have inspect in the namespace (public no less) which is an intermediate multimethod. Renaming all those methods to something else is possible but it's easier to leave this as is.

Fair enough. I didn't notice the existing inspect multimethod.