-
-
Notifications
You must be signed in to change notification settings - Fork 645
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
[Fixes #3170] Skip ensure of repl available on xref functions #3171
Conversation
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.
Looks like it would work!
Did you get to try it? (Of course, without any other workaround in place)
cc6f5d9
to
65bede2
Compare
Yes @vemv, I called that function and it did return |
Thank you! |
🍻! Feel free to propose bumping the commit in the Doom repo |
that's what I just did! 😄 |
Just in case, I meant upstream e.g. doomemacs/doomemacs#6183 |
yes, I was talking with Henrik on Doom's discord, he said will bump cider soon with this fix :) |
awesome! |
@@ -234,7 +234,7 @@ thing at point." | |||
"Used for xref integration." | |||
;; Check if `cider-nrepl` middleware is loaded. Allows fallback to other xref | |||
;; backends, if cider-nrepl is not loaded. | |||
(when (cider-nrepl-op-supported-p "ns-path") | |||
(when (cider-nrepl-op-supported-p "ns-path" 'skip-ensure) |
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.
This change had an oversight fixed in 3627780
I'm not sure this was the right approach to solving the problem, but you merged it before I saw the PR. Not that big of deal in general, but I was thinking that actually the predicate function should always return |
(likely the behavior you're trying to fix is somewhat accidental, given where it's coming from) |
Yeah, I like precicate functions without side-effects, your idea sounds better indeed, but probably will require a little bit more refactor I think |
We can always simply check if the test suite works fine if we make this the default behavior (suppressing the exception from |
Yeah that wasn't ideal, sorry, I guess I wanted to leave it bug-free for Doom/lsp users asap. Given this 'api' has not been shared as a tagged release, I'd be happy for the solution to be changed if that's better. |
Fixes #3170 adding a extra arg to skip the ensure for xref functions.
Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):
eldev test
)eldev lint
) which is based onelisp-lint
and includescheckdoc
, check-declare, packaging metadata, indentation, and trailing whitespace checks.Thanks!