-
-
Notifications
You must be signed in to change notification settings - Fork 649
cider-ns-refresh
: jump to the relevant file/line on errors
#3627
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
Conversation
cider-ns.el
Outdated
@@ -219,10 +235,11 @@ indirectly load via require\"." | |||
|
|||
;;;###autoload | |||
(defun cider-ns-refresh (&optional mode) | |||
"Reload modified and unloaded namespaces on the classpath. | |||
"Reload modified and unloaded namespaces, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first line should always be a completely sentence.
Also - we'll have to update this again after the clj-reload
PR gets merged, as I noticed their logic for deciding what to reload is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you generously suggest a specific wording?
Reload modified and unloaded namespaces on the classpath
isn't always accurate.
Maybe we can kill two birds in one stone and say:
Reload code according to the underlying code loading lib logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of something like Reload namespaces in a smart way.
or something along those lines. I think this function predates cider-ns-reload
, so probably we should mention in the docs that cider-ns-refresh
is basically a more powerful version that relies on external tools to work or something along those lines.
I'm thinking of later aliasing this to cider-ns-smart-reload
or something similar, as I think the "refresh" terminology is a bit confusing at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Another way to word smart
(which I don't oppose but might be ambiguous) is to refer to the "reloaded workflow".
Probably the "reloaded workflow" concept transcends a specific tool (https://www.cognitect.com/blog/2013/06/04/clojure-workflow-reloaded describes Component, but doing it with Integrant also belongs to "reloaded").
It's generally well-understood that one either does a pure "repl-driven development", or is in the "reload workflow" camp.
(Or does both, like a few of us 😄)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with mentioning the reloaded workflow in the docstring and the user docs, but I think it would make for very long function and var names. :-)
cider-ns.el
Outdated
@@ -169,7 +169,23 @@ namespace-qualified function of zero arity." | |||
(goto-char (point-max)))) | |||
|
|||
(when (member "error" status) | |||
(cider--render-stacktrace-causes error)))) | |||
(let* ((buf) | |||
(jump-args (seq-some (lambda (cause-dict) ;; a dict representing an exception cause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to extract this to a named helper function, so it's clearer what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the buf
mutation it wasn't possible to extract just the lambda, however I did extract the whole defun.
All done - thanks! You might want to introduce some |
Fixes #3626
Tested out - works nicely.
Cheers - V