Stacktrace at point or in region #758

r0man · 2022-09-18T18:03:00Z

This PR adds Stacktrace at point or in region support.

Please see: clojure-emacs/orchard#164

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 README (if adding/changing middleware)

bbatsov · 2022-09-19T05:30:37Z

Looks good, failing CI aside.

r0man · 2022-10-09T14:46:49Z

src/cider/nrepl.clj

-                            :optional wrap-print-optional-arguments
+                            :optional (assoc wrap-print-optional-arguments "stacktrace" "The stacktrace to be parsed and analyzed as a string.")


@bbatsov @vemv I am wondering if I should a new "parse stacktrace" op instead off adding this new optional parameter? It feels cleaner instead of hijacking the existing op to me. Wdyt?

I agree. Generally we've been using action-like names for most ops (e.g. eval, clone, etc), so the stacktrace op was named somewhat poorly in that regard anyways, as it was super clear what does it do by its name alone.

r0man · 2022-10-14T07:35:47Z

src/cider/nrepl.clj

@@ -437,7 +437,11 @@ Depending on the type of the return value of the evaluation this middleware may
    :expects #{}
    :handles {"stacktrace" {:doc "Return messages describing each cause and stack frame of the most recent exception."
                            :optional wrap-print-optional-arguments
-                            :returns {"status" "\"done\", or \"no-error\" if `*e` is nil"}}}}))
+                            :returns {"status" "\"done\", or \"no-error\" if `*e` is nil"}}
+              "parse-stacktrace" {:doc "Parse the `stacktrace` and return messages describing each cause and stack frame."


@bbatsov I added a new op called parse-stacktrace. Are you fine with this name?

Hmm, I think I rename it to analyze-stacktrace ...

bbatsov · 2022-10-20T08:12:38Z

src/cider/nrepl.clj

+                                    :requires {"stacktrace" "The stacktrace to be parsed and analyzed as a string."}
+                                    :optional wrap-print-optional-arguments
+                                    :returns {"status" "\"done\", or \"no-error\" if `stracktrace` is not recognized"}}
+              "stacktrace" {:doc "Return messages describing each cause and stack frame of the most recent exception."


We can also add some alias for this op with a more descriptive name - e.g. analyze-last-stacktrace or something like this. Or we can consider the new name an alias of the old one and make them behave the same way with/without an explicit stacktrace.

I think I like analyze-last-stacktrace but how would you do this alias? Just adding a new op and pointing to the same implementation? How do we deprecate the old one?

I think your second suggestion is what I had before (minus the rename). If the stacktrace parameter is present it would parse and analyze the value from the parameter, otherwise use the last exception when present. I think I would prefer separating those 2 ops, but up to you, I'm also fine with the optional parameter.

I think I like analyze-last-stacktrace but how would you do this alias? Just adding a new op and pointing to the same implementation? How do we deprecate the old one?

Yeah, just a new pointing to the same implementation. At some point it might be nice to add better support for this to nREPL itself. As for the deprecation - some note in the docs will do. You can also use notify-client to send them deprecation messages.

@bbatsov I added the analyze-last-stacktrace op and made the stacktrace op deprecated. The deprecation is reflected in the docs and the stacktrace op also does send a notification to the user. I am not sure if this is a good idea, because I had to make a change in Cider to handle this notification in the response. See here:
clojure-emacs/cider@0d0633a#diff-e5e7f12a3638dc838dbd4f61a5bd797efc0ac6120e58c80ec1a92970eea3a27cR479

I don't know which clients we have, but if they don't handle notification messages in a stacktrace response they might break. So I would suggest removing the client notification to not break anyone and just mark the stacktrace op deprecated in the docs. WDYT?

Fair point about some clients probably not supporting this (I'm surprised we forgot to add it to CIDER :D ), so I guess we can just rely on a deprecation in the docs.

vemv · 2022-10-23T11:46:05Z

project.clj

@@ -8,7 +8,8 @@
            :url "http://www.eclipse.org/legal/epl-v10.html"}
  :scm {:name "git" :url "https://github.com/clojure-emacs/cider-nrepl"}
  :dependencies [[nrepl "1.0.0"]
-                 ^:inline-dep [cider/orchard "0.10.0" :exclusions [com.google.code.findbugs/jsr305 com.google.errorprone/error_prone_annotations]]
+                 ^:inline-dep [cider/orchard "0.0.0" :exclusions [com.google.code.findbugs/jsr305 com.google.errorprone/error_prone_annotations]]


What's 0.0.0?

(sorry if I missed it from a possible previous conversation)

Normally, locally I just run make install on the Orchard project and I still get to use 0.10.0 (i.e. the unaltered version) with local changes applied

Oops, that was a mistake. Yeah, this was me trying this locally. I changed it back.

Or actually I bumped it to the next orchard version.

This commit changes the Mr Anderson project prefix from cider.nrepl.inlined-deps to cider.nrepl.inlined.deps. This avoids issues with renaming fully qualified record references. See benedekfazekas/mranderson#73

r0man · 2022-11-26T12:12:57Z

@bbatsov @vemv This one would be ready from my side.

bbatsov · 2022-11-26T21:45:12Z

After merging it I noticed we forgot to update the changelog.

bbatsov · 2022-11-26T21:48:21Z

It also seems to me you've updated ops.adoc manually, instead of generating it from the middleware descriptors. This will have to be fixed, otherwise your nice docs will disappear once this is regenerated with the lein docs task.

r0man · 2022-11-27T09:46:10Z

Oops, I opened another PR to address those issues: #762

r0man mentioned this pull request Sep 18, 2022

Add cider-stacktrace-analyze-in-region clojure-emacs/cider#3247

Merged

6 tasks

r0man commented Oct 9, 2022

View reviewed changes

r0man commented Oct 14, 2022

View reviewed changes

bbatsov reviewed Oct 20, 2022

View reviewed changes

vemv reviewed Oct 23, 2022

View reviewed changes

r0man added 13 commits November 26, 2022 12:54

Move stacktrace analyzer to orchard

8978e0b

Add stacktrace parameter

76744bd

Fix unused binding issues

6dd4658

Add parse-stacktrace op

fcf4235

Add op to ops.adoc

f6877b9

Format

4cf0fbf

Add blank

bb205d5

Convert frame to data with throwable/StackTraceElement->vec

df32184

Rename op

d66967b

Add analyze-last-stacktrace op, deprecate stacktrace op

068f988

Don't use dash in Mr Anderson project prefix

a41569e

This commit changes the Mr Anderson project prefix from cider.nrepl.inlined-deps to cider.nrepl.inlined.deps. This avoids issues with renaming fully qualified record references. See benedekfazekas/mranderson#73

Use stacktrace parser and analyzer from haystack

867b55f

Update haystack version

cce1aac

r0man force-pushed the stacktrace-at-point branch from 396c48a to cce1aac Compare November 26, 2022 11:56

bbatsov merged commit 1fa95f2 into clojure-emacs:master Nov 26, 2022

r0man deleted the stacktrace-at-point branch November 27, 2022 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stacktrace at point or in region #758

Stacktrace at point or in region #758

r0man commented Sep 18, 2022 •

edited

Loading

bbatsov commented Sep 19, 2022

r0man Oct 9, 2022

bbatsov Oct 10, 2022

r0man Oct 14, 2022

r0man Oct 14, 2022

bbatsov Oct 20, 2022

r0man Oct 20, 2022

bbatsov Oct 21, 2022

r0man Oct 23, 2022

bbatsov Oct 25, 2022

vemv Oct 23, 2022

r0man Oct 23, 2022

r0man Oct 23, 2022

r0man commented Nov 26, 2022

bbatsov commented Nov 26, 2022

bbatsov commented Nov 26, 2022

r0man commented Nov 27, 2022

		:optional wrap-print-optional-arguments
		:optional (assoc wrap-print-optional-arguments "stacktrace" "The stacktrace to be parsed and analyzed as a string.")

Stacktrace at point or in region #758

Stacktrace at point or in region #758

Conversation

r0man commented Sep 18, 2022 • edited Loading

bbatsov commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0man commented Nov 26, 2022

bbatsov commented Nov 26, 2022

bbatsov commented Nov 26, 2022

r0man commented Nov 27, 2022

r0man commented Sep 18, 2022 •

edited

Loading