Kyle Meyer <k...@kyleam.com> writes: > 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.
Cool, will do. I was concerned that the lexical-binding stanza was missing for a reason, but I'll give it a shot. > Also, why use > > (doc-funs (symbol-value 'eldoc-documentation-functions)) > > rather than > > (doc-funs eldoc-documentation-functions) > > ? Good question...I was doing that just because the original code does ~(symbol-value 'eldoc-documentation-function)~ and I assumed there was some reason I didn't understand for doing so. >> + (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. Oops, will do. >> -(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). Good call, will do. > 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). I was just thinking more about this, and I'm concerned I might need to change things around a little bit more. The closure that org-eldoc-get-mode-local-documentation-function now returns under Emacs 28 doesn't take any arguments and instead lets the eldoc-documentation-strategy function create its own new callback. I think the current approach will appear to work when the documentation function returns a value directly, but if it uses the callback, then I think that it won't, since it invokes a new, separate callback. I will take another crack at addressing this and send another patch shortly. > Thanks again for working on this. My pleasure!