-
-
Notifications
You must be signed in to change notification settings - Fork 643
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
Modified xref-backend-references
to use buffer locations instead of filenames
#3349
Conversation
They are relative to |
Am I correct in thinking that I should convert filenames using that information instead of the proposed fix in the PR? |
Yes, most likely it's better to fix The relevant files are: Let us know if you're up for a PR there, else I could eye it this week/end |
I'm looking at contributing to cider-nrepl, but I'm having problems loading a locally installed version of |
Not that I recall. If the version you are If you hack on Orchard, Hope it helps, else let us know of any specific problem |
I would rather use the |
Fine by me. |
I guess it's fine, I'd just keep an eye on Also, if there's truly an issue cider-nrepl side, fixing it there would fix it for non-Emacs clients. |
Yep. Probably we should eventually have the middleware return absolute paths, but I think it's preferable short-term to go with any fix instead of leaving this functionality broken. I've been super busy in the past few weeks, otherwise I would have tackled this myself. |
@@ -280,7 +280,13 @@ thing at point." | |||
(let* ((filename (nrepl-dict-get info "file")) | |||
(column (nrepl-dict-get info "column")) | |||
(line (nrepl-dict-get info "line")) | |||
(loc (xref-make-file-location filename line column))) | |||
(buf (cider--find-buffer-for-file filename)) | |||
(loc (xref-make-buffer-location buf (with-current-buffer buf |
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'd add some comment here explaining the need for this and some todo to eventually use xref-make-file-location
once the nREPL api has been modified.
@jeeger Don't forget to add a changelog entry for this bugfix. |
Mmm, I thought @jeeger would look into cider-nrepl/Orchard. Are we sure that If we absolutely want cider-nrepl fixed, I can prioritize it for the first half of this week. I was simply waiting for @jeeger input in the meantime. |
Probably both things can be taken care of, nrepl side (assuming there's an issue there) and cider.el side.
Ideally we could decouple cider-find-file's logic (finding the right thing) from the side-effect (producing a buffer). If it was proven too difficult, maybe we could close the buffers that weren't open already prior to the computation? Worth pointing out, Emacs sessions can last weeks, if buffers piled up to e.g. 1000, it wouldn't surprise me that that cascaded into all sorts of issues. Which is to say, better to tread carefully. |
I have to admit I still use grep instead of xref, so for me that's not a big issue. 😆 |
I was looking into orchard/cider-nrepl, but I didn't get anywhere last week — turns out that the |
9f81132
to
c72141e
Compare
… filenames The filenames returned by `cider-sync-request:fn-refs` were not in the format expected by uxref, probably because they are relative to the project root and not relative to the current buffer (which is what xref needs, from my understanding). I've implemented a somewhat dirty fix by using identical code to `xref-backend-definitions`, which uses `cider--find-buffer-for-filename`. This function seems to work around the issue, with the drawback of potentially opening a large amount of buffers. The cleaner way to fix this bug would be computing absolute file names, but I am unsure what the filenames returned by `cider-sync-request:fn-refs` are relative to.
c72141e
to
fbe7000
Compare
That's useful info, thanks! So as suggested in #3349 (comment) I think hacking Either:
...or:
Does this sound like a workable plan? Thanks - V |
I'll work based on the design outlined at #3349 (comment) soon, if there are no objections. |
No objections from me. |
new PR soon Thanks everyone for the valuable input! |
The final fix took some inspiration from here, with nuances, and also some changes at cider-nrepl level. Things should now be pretty for all things xref:
Feel free to try them out in CIDER master. Thanks again! |
The filenames returned by
cider-sync-request:fn-refs
were not in the format expected by uxref,probably because they are relative to the project root and not relative to the current buffer (which
is what xref needs, from my understanding). I've implemented a somewhat dirty fix by using identical
code to
xref-backend-definitions
, which usescider--find-buffer-for-filename
. This functionseems to work around the issue, with the drawback of potentially opening a large amount of buffers.
The cleaner way to fix this bug would be computing absolute file names, but I am unsure what the filenames returned by
cider-sync-request:fn-refs
are relative to.Fixes #3112.