-
-
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
Avoid cider--error-phase-of-last-exception
recursive loop
#3607
Conversation
(lambda (buffer err) | ||
(cider-emit-interactive-eval-err-output err) | ||
|
||
(let ((phase (cider--error-phase-of-last-exception buffer))) | ||
(let ((phase (if error-phase-requested |
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 is the main fix
I could only immediately reproduce it for cider-interactive-eval-handler
. But I used the same pattern in the other handlers as it's easy enough, and harmless.
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 add some comment explaining the need for the toggling between t/nil
of this flag, as it's definitely not going to be obvious to most people.
(cider--nrepl-print-request-map fill-column)) | ||
(seq-mapcat #'identity)) | ||
conn | ||
'abort-on-input ;; favor responsiveness over this feature, in case something went wrong. |
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.
When I reproduced the issue, 'abort-on-input made it less terrible, so it seems wise to leave this here.
@@ -781,7 +782,10 @@ The handler simply inserts the result value in BUFFER." | |||
(when (and source-buffer | |||
(listp bounds)) ;; if it's a list, it represents bounds, otherwise it's a string (code) and we can't display the overlay | |||
(with-current-buffer source-buffer | |||
(let* ((phase (cider--error-phase-of-last-exception buffer)) | |||
(let* ((phase (if error-phase-requested |
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.
It seems this if
is duplicated a few times, so it might be good to make it some utility function instead.
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.
Not sure if an abstraction could capture the necessary pattern
At most I can imagine a macro that ensured that a local variable error-phase-requested
exists.
conn))) | ||
(nrepl-dict-get result "phase")))))) | ||
(let ((nrepl-err-handler (lambda (&rest _))) ;; ignore any errors during this request to avoid any recursion | ||
(nrepl-sync-request-timeout 4)) ;; ensure that this feature cannot possibly create an overly laggy UX |
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.
Why 4? That's a oddly specific number. :-)
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.
4 - ample enough to give it time to be slow (especially if there was some sort of cache to be hit), tight enough to not negatively impact the UX
I'd be fine with 3 or 5 as well 😄
Apart from my small remarks the changes look reasonable to me. |
Thanks for the review! I was hesitant about an abstraction (even more so with #3606), but if you felt it was needed we could keep it in the tracker. |
No, it's not a big deal - I'm just always wary of repeated code, as usually we get to a situation where someone forgets to make some change at every location. Anyways, I doubt this code is going to see much change going forward. |
This was an extra measure included as part of #3607 It resulted in `*cider-error*` not being shown, in certain scenarios. That particular measure wasn't critical in avoiding the issue tackled in that PR.
This was an extra measure included as part of #3607 It resulted in `*cider-error*` not being shown, in certain scenarios. That particular measure wasn't critical in avoiding the issue tackled in that PR.
Fixes #3605
Cheers - V