James N. V. Cash writes: > Kyle Meyer <k...@kyleam.com> writes:
>> Okay, so when eldoc-documentation-functions is defined (Emacs >=28), we >> take the first function and go with it. That might not be exactly what >> you'd see in the native buffer, depending on whether there are other >> members of eldoc-documentation-functions and how they interact. (I'm >> being vague, because I don't really know anything about eldoc, but it >> seems like that must be the case.) Anyway, I'd guess it will be good >> enough in most cases, and it's certainly better than the recursion >> error. > > Ah yes, very true. I've attached another patch, which tries to better > preserve how the new eldoc strategy works, by passing through the > callback to the mode-local eldoc function if available, which will be a > closure over the configured documentation strategy with > eldoc-documentation-functions bound to the appropriate mode-local value. Thanks, sounds good. > Subject: [PATCH] Address org-eldoc-recursion issue [...] > @@ -116,8 +116,13 @@ > (when (fboundp mode-func) > (with-temp-buffer > (funcall mode-func) > - (setq doc-func (and eldoc-documentation-function > - (symbol-value > 'eldoc-documentation-function))) > + (setq doc-func (if (boundp 'eldoc-documentation-functions) > + (lexical-let ((doc-funs (symbol-value > 'eldoc-documentation-functions))) Using lexical-let here is problematic because it's obsolete since Emacs 24. Taking a quick glance, I don't see any issues with switching this file over to lexical binding by adding " -*- lexical-binding: t; -*-" to the first line. Also, why use (doc-funs (symbol-value 'eldoc-documentation-functions)) rather than (doc-funs eldoc-documentation-functions) ? > + (lambda () > + (let ((eldoc-documentation-functions > doc-funs)) > + (funcall eldoc-documentation-strategy)))) > + (and eldoc-documentation-function > + (symbol-value > 'eldoc-documentation-function)))) nit: Please switch this to the Elisp style of indenting the `else' arm less than the `then' arm. > (puthash lang doc-func org-eldoc-local-functions-cache)) > doc-func) > cached-func))) > @@ -127,7 +132,7 @@ > (declare-function php-eldoc-function "php-eldoc" ()) > (declare-function go-eldoc--documentation-function "go-eldoc" ()) > > -(defun org-eldoc-documentation-function (&rest _ignored) > +(defun org-eldoc-documentation-function (&optional callback) Perhaps even with the callback parameter the &rest should stay around. The docstring of eldoc-documentation-functions makes me nervous because it says "each hook function is called with _at least_ one argument" (my emphasis). > "Return breadcrumbs when on a headline, args for src block header-line, > calls other documentation functions depending on lang when inside src > body." > (or > @@ -161,7 +166,12 @@ > (string= lang "golang")) (when (require 'go-eldoc nil t) > (go-eldoc--documentation-function))) > (t (let ((doc-fun > (org-eldoc-get-mode-local-documentation-function lang))) > - (when (functionp doc-fun) (funcall doc-fun)))))))) > + (when (functionp doc-fun) > + (if (functionp callback) > + (condition-case nil > + (funcall doc-fun callback) > + (wrong-number-of-arguments (funcall doc-fun))) > + (funcall doc-fun))))))))) Hmm, I think the more complete approach you put together for org-eldoc-get-mode-local-documentation-function, along with your change to consider the callback parameter here, means we don't need to bother with the condition-case/wrong-number-of-arguments dance. The callback alone should be a reliable indication we're on Emacs 28, in which case we can expect the function to accept a callback argument (even if they ignore it like python-eldoc-function does). Thanks again for working on this.