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

Fix broken stacktrace navigation to references of the form foo/eval16959/fn #854

Merged
merged 2 commits into from
Feb 1, 2015

Conversation

vspinu
Copy link
Contributor

@vspinu vspinu commented Oct 14, 2014

I closed #850 prematurely and cannot reopen it now. The problem is not fixed. Sometimes info middleware doesn't return file location. In those cases just pick the file from stacktrace.

;; stacktrace returns more accurate line numbers
(info (nrepl-dict-put info "line" (button-get button 'line))))
(info (nrepl-dict-put info "line" (button-get button 'line)))
;; info middleware returns full paths, but sometimes fails
Copy link
Member

Choose a reason for hiding this comment

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

"sometimes fails" is a bit confusing. If there's some problem with the middleware we should fix it there instead of doing a workaround here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example that I have in mind is indeed has to do to the error in the middleware but with the old code user will not notice that because the message "No source location found" overshados the middleware error.

With the new code, you are taken to the location but the error in the middleware is also displayed.

Copy link
Member

Choose a reason for hiding this comment

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

The middleware error should also be visible in the REPL buffer, so users are unlikely to miss it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bozhidar Batsov on Fri, 17 Oct 2014 12:27:49 -0700 wrote:

The middleware error should also be visible in the REPL buffer, so users are
unlikely to miss it.

I was missing it till I started investigating what was going on. My repl
is full of my own user errors so I am completely ignoring it.

@vspinu vspinu force-pushed the stack-nav branch 2 times, most recently from 83dbbe3 to 6c35f1a Compare February 1, 2015 15:48
  Java stacktrace reports lines even for methods of inner classes (seen as
  foo.bar/evalNNNN in stacktrace). `info` middleware doesn't return location for
  these methods and there is no easy way to access it as the following post
  explains:

     http://stackoverflow.com/questions/7483421/how-to-get-source-file-name-line-number-from-a-java-lang-class-object
@vspinu
Copy link
Contributor Author

vspinu commented Feb 1, 2015

All right. This patch is still needed even after fixing the middleware error in clojure-emacs/cider-nrepl#142..

I guess you would like a changelog for this as well. Now the problem is that all my 3 pulls have changelog entry and will conflict. /This is one of the reasons I hate changelog :-)/.

So let's do like this, once all the PR's are closed I will add a change log for all of the changes. Otherwise I have to rebase all pulls on top of each other or synchronise with you and make it iteratively.

@bbatsov
Copy link
Member

bbatsov commented Feb 1, 2015

I guess you would like a changelog for this as well. Now the problem is that all my 3 pulls have changelog entry and will conflict. /This is one of the reasons I hate changelog :-)/.

Yes, this is the only significant downside, but I'm used to this already. :-)

So let's do like this, once all the PR's are closed I will add a change log for all of the changes. Otherwise I have to rebase all pulls on top of each other or synchronise with you and make it iteratively.

Fair enough.

I see a typo in the second commit and to be honest I totally forgot what this PR was supposed to fix.

@vspinu
Copy link
Contributor Author

vspinu commented Feb 1, 2015

I see a typo in the second commit and to be honest I totally forgot what this PR was supposed to fix.

(defn tt []
  (/ 1 0))

(tt)

Eval with source tracking command and then go to the line with /evalNNN in it. The source will not be found. Comit message describes the technicalities.

@vspinu
Copy link
Contributor Author

vspinu commented Feb 1, 2015

I see a typo in the second commit

I don't see any.

bbatsov added a commit that referenced this pull request Feb 1, 2015
Fix broken stacktrace navigation to references of the form `foo/eval16959/fn`
@bbatsov bbatsov merged commit d309a99 into clojure-emacs:master Feb 1, 2015
@bbatsov
Copy link
Member

bbatsov commented Feb 1, 2015

I don't see any.

My bad. I keep forgetting most people use American spelling (favor seemed incorrect to me)...

@vspinu vspinu deleted the stack-nav branch August 1, 2017 10:47
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.

2 participants