Add Datafy and Nav support to the inspector #161
Conversation
|
Oops, going to look at those failing tests soon. |
|
Thanks for the follow-up commits! Do you need help with the build, or you got it? Either way, ideally the tests would fail as clearly as possible. I'd suggest, please use the following technique (in addition to preferring files to large inline strings): (or (is (= expected actual))
(do
(println (format "Expected: the contents you can find in %s.txt" path))
(println (format "Actual: %s" actual))
(println (format "In order to obtain a diff, you can paste the Actual value in %s.txt and leverage `git diff`"))))This will make future maintenance more pleasant. |
|
Hi @vemv , thanks for your review. I think I addressed most of it. I did not go so far as to move the data to files, but I changed them to compare data instead of the brittle strings. Is that ok for you? That helped me also to find some CI issues, with method signatures being printed differently across JDKS (some had native in them, some not). Still digging into it ... |
|
Oh, I forgot. The text snippets I posted above are actually taken from the Cider Inspect buffer. I got it running locally and was viewing the results in Cider. :) They seem to look and work as I would expect. I'm actually not a REBL user myself. So, if anyone (especially someone familiar with REBL and the Cider inspector) has suggestions for improvements, feedback welcome! |
|
I think I'm fine now. The tests are passing now. Thanks for your help! |
r0man
force-pushed
the
datafy
branch
2 times, most recently
from
dd813c4
to
7e3cdba
Compare
r0man
force-pushed
the
datafy
branch
3 times, most recently
from
2923db3
to
8ab4cde
Compare
test/orchard/inspect_test.clj
Outdated
| inspect | ||
| render))))) | ||
| (is (= (if datafy? | ||
| '("Class" |
There was a problem hiding this comment.
Looks like you can use (cond-> '["Class" ,,,] datafy? (conj ,,,)) for DRY
| (testing "doesn't show path if unknown navigation has happened" | ||
| (is (-> long-map inspect (inspect/down 40) (inspect/down 0) (inspect/down 1) render ^String (first) (.endsWith "(:newline))")))) | ||
| (is (= '(:newline) (-> long-map inspect (inspect/down 40) (inspect/down 0) (inspect/down 1) render last)))) |
There was a problem hiding this comment.
I like these new assertions, they're simpler.
Just in case, why is first now last?
There was a problem hiding this comment.
Previously render returned a vector with a single string. So what the test did, it used first to get at the string, and then matches that large string with endsWith. Since we now return a seq, extracting the first element is not needed (we are interested in the last one). The test diff looks kind of confusing
test/orchard/inspect_test.clj
Outdated
| (.setStackTrace (into-array StackTraceElement []))) | ||
| inspect render skip-to-datafy-section)] | ||
| ;; Differences in JDKs :/ | ||
| (is (or (= '("Datafy: " |
There was a problem hiding this comment.
Let's code it approximately like this instead, leaving something with fixed expectations:
(case misc/java-api-version
8 (is (= ,,,))
(11 16 17) (is (= ,,,))
(assert false "Not clause for this JDK"))
There was a problem hiding this comment.
I think these make sense, however the PR and changelog should be clear about what "Datafy and Nav" support mean.
In the PR they mean rendering more data at the bottom.
But what would be more commonly understood as "datafy and nav support" would be to:
- use the nav protocol to navigate objects
- while cider presumably has its own way to navigate objects (e.g. we already can recursively walk down a nested vector), nav would make even more objects navigable
- use datafy as the only source of data
- e.g. replace other fields.
I'd say REBL works like this.
I'm not saying it should be done, but I feel it should be part of the conversation.
|
@vemv I believe the PR makes more objects available to the (defn- datafy-nav-data-tx [obj]
(comp (map (fn [[k v]] (some->> (nav obj k v) datafy (vector k))))
(remove nil?)))
(defn- datafy-nav-data [obj]
(let [data (datafy obj)]
(if-not (map? data)
data
(into {} (datafy-nav-data-tx obj) data))))Notice the call to I previously had 2 sections, one called "Datafy Contents:" and What you mentioned in "use datafy as the only source of data" is I didn't want to completely change the way the inspector |
Yeah, but it does just one level deep down (i.e. it's not fully recursive). Users might want to further navigate. Or not at all! So rendering text is sort of a best-effort guess of what can be useful to users. And then users, if interested in this text, cannot navigate it directly, but they have to use the traditional CIDER navigation instead. This mixing-and-matching of two approaches might create bit of a strange, possibly unintuitive hybrid. However, as said, I'm not against it - it's displaying information/insights that may not be discoverable at all otherwise. So I'd say, let's go for your PR, but long-term someone should give a good think to what the UX should be - it has to be created holistically between orchard and cider.el. We might even end up with two independent features: And lastly, I'll let @bbatsov chime in before any merging. Cheers - V |
|
@vemv Another option would be to define the And use it in all places where we inspect a value. If datafy is This would change the current user interface a bit. So, without that After the change I describe it would look like this: Inspecting a Datomic entitiy seq would then look like this: (entities my-db ::commit/sha)And navigating into one of the entities would look like this: Wdyt? |
|
I see! I wouldn't be on board with it as-is. For instance, for: The formatting isn't particularly great. And Without that optional, yet full recursivity that is triggered by user actions on-demand, we lose all the 'magic' of the rebl. (if we had said magic, formatting would matter less, but we cannot have the worst of both worlds) |
|
My preference would also be to not change the default behavior. On CIDER's front we can expose the Datafy and Nav support as opt-in. |
|
Btw, might be a good idea to document the user-interface somewhere in the namespace itself and in the README of Orchard. (you can reuse the description from the PR) We've always been pretty light on docs and we should start changing this at some point. |
Just in case - this PR just adds extra paragraphs with Datafy/Nav info, but otherwise does not change behavior. |
|
I saw this and I meant that we should mention somewhere that if you're using Clojure 1.10 and if the representation is different you'll see different inspector output. Going forward we can add there how to just use Datafy by default if you want to. Obviously the bigger point here is that the workings of the inspector are not really documented. :( Still, we should start somewhere. |
|
(and the API docs, that we point people to, don't really exist :D ) |
|
@vemv @bbatsov Regarding this fully recursive navigation. This is how inspecting a namespace looks for example: If you have point on the map next to "Imports:" or ":imports" in the If you have point on any text inside the map, on the java.lang.Enum The Slime inspector also works how Cider does it at the moment, right? |
|
Also, what do you think about some UI tweaks. What about rendering the The difference is to place the "---" in front of the section name. To compare take a look at the PR description, there you can see how we render it at the moment. This would be more consistent with how we do when rendering a class We could align the "Refer from" in the namespace: It was not clear to me how to use "---" consistently. Some are in Wdyt? |
|
I like the proposal!
There is a lot of legacy here and little design thought I guess. :-) The first version of the inspector mostly followed the original inspector UI from SLIME for Common Lisp and it lived in |
More or less. I haven't used SLIME in ages, so I don't remember the details at this point. |
Yeah, that's what I had in mind + some "design" documentation explaining at high level the workings of the inspector and why it might be useful to people. |
|
I updated the inspector.org document with the UI tweaks I proposed
Here's a link to the new proposal: https://github.com/r0man/orchard/blob/datafy/doc/inspector.org And here is how it looked like previously: I haven't updated the tests yet, and haven't committed the code for I also experimented a bit with @vemv's suggestion about moving the WDYT about those changes? Are you ok with them, do you have any |
|
The updated format looks good to me. It seems you've also updated the tests in the mean time. |
|
Alright, I'm trying to improve the tests a bit and also do some fine tuning. I also run into an issue the way I rendered namespaces. I might need to tweak that also a bit. I'll ping you when I'm ready. |
|
I'd also suggest starting the inspector docs with some general design notes (e.g. rationale behind some decisions, tradeoffs we've made, etc - similar to the explanations from the PR itself). |
|
As a quick note I don't have much bandwidth these days so I'll unsubscribe from this thread. Either way, kudos for the great work and driving it towards the finish line! |
Alright, thanks for your help so far! |
|
Hi @bbatsov, I added more information to the inspector docs, updated the changelog and link to the docs from the readme. Can you have another look please? |
|
The PR looks good to me now. Fantastic work! 🙇♂️ I guess the next step would be to cut a new release and get the new version in |
|
Hi @bbatsov, yes, that's right. It should work out of the box without any changes to cider-nrepl and cider itself. |
|
@r0man The new Orchard is out, but it seems the changes to the format did break 20 unit tests in cider-nrepl https://app.circleci.com/pipelines/github/clojure-emacs/cider-nrepl/608/workflows/0e81dbda-0ca4-405b-b82a-f69ae3f1d432/jobs/6270 Please, look into fixing them when you can. |
|
Oops, sorry about that. I'll fix them |
Hello,
this PR adds basic support for Datafy and Nav to Orchard's
Inspector. The PR adds a Datafy section to the inspection view.
A good introduction to Datafy and Nav can be found here:
https://corfield.org/blog/2018/12/03/datafy-nav/
User Interface
Here's an overview how the UI looks like at the moment. In a nutshell,
a Datafy section is added to an object if it's datafy representation
is different than the original object.
This is done because datafy is defined on Object. Otherwise we would
show the same data in the datafy section as has been shown in a
previous section.
Class
References
Namespace
Objects extended with metadata
Nav support
Throwable support
Datomic Entity Maps
With the following code Datomic entities can be inspected and navigated.
A list of Datomic entities looks like this in the inspector.
Inspecting one of those entities looks like this:
I haven't opened a PR on cider-nrepl yet, because I was constantly
tweaking the rendering a bit. Once we aggree on a UI I can do that
works as well (if there needs to be done anything).
WDYT?