bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#41531: 27.0.91; Better handle asynchronous eldoc backends


From: João Távora
Subject: bug#41531: 27.0.91; Better handle asynchronous eldoc backends
Date: Sat, 04 Jul 2020 12:00:57 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)

Eli Zaretskii <eliz@gnu.org> writes:

>> Anyway I think my efforts are ready to push to master.  I'll do so soon
>> unless someone raises a serious problem about it.
>
> Not a serious problem, but some comments:
>
>> -(defun eldoc-message (&optional string)
>> +(make-obsolete
>> + 'eldoc-message "use `eldoc-documentation-functions' instead." "1.1.0")
>
> Isn't the version part of the obsolete message supposed to tell the
> version of Emacs?

Yes, but eldoc.el is its own versioned piece of software now, because it
is a GNU Elpa :core package.  So I opted to use that versioning instead.
Maybe I shouldn't have?  Don't know if there's prior art for this
situation, but I'm reasonably confident I personally have taken this
decision before.

> The change to the number of arguments of the functions in the
> eldoc-documentation-functions hook is backward-incompatible, isn't it?
> I see you've changed the relevant functions in our sources, but what
> about 3rd-party packages?

I've checked and there were none.  The eldoc-documentation-functions
variable came to be in Emacs master: it's not in Emacs 27.  I myself
"released" eldoc.el to GNU Elpa almost 2 months ago (see
9ebf51999ce58cccc2a228fd07a18c7b472dd3fd).  I had intended to streamline
that release together with these async-related developments, but reality
(and mostly Dmitry :-) intervened.

So indeed there is the danger that, in the intervening 2 months, some
3rd party package depending on GNU Elpa's Eldoc, started using the
argless eldoc-documentation-functions, and will be surprised by this
backwards-incompatible change.  However, I think the changes of that are
unlikely.  I searched Github and Google for such references and couldn't
find any.

So I think it's safe.  (But even if it weren't, I'd still argue for the
change + fallout ).

> Also, the doc string of this hook needs clarification regarding the
> arguments: it first says that CALLBACK is the only mandatory argument
> to the hook functions, but then, out of the blue, appear additional
> arguments DOCSTRING and a list of key-value pairs.  Confusing.

Understood.

> The doc strings have some words in UK English spelling "(e.g.,
> "honour"), please fix that.  Also, please make sure comments all start
> with a capital letter, end with a period, and comprise full English
> sentences.

OK.

> The doc string of eldoc-documentation-compose doesn't say a word about
> the function's argument.

That function isn't meant to be called by users, and the EAGERLYP is a
code-saving trick.  But of course I should document it.

> In the doc string of eldoc-documentation-strategy, you use the phrase
> "queries the special hook for all functions that produce doc strings"
> to mean, AFAIU, that the specified functions in the hook-variable list
> are called.  IMO, this wording could be confusing; suggest to use this
> instead:
>
>   `eldoc-documentation-compose': calls all the functions in the hook,
>   and displays all of the resulting doc strings ...

This of course, assumes that people know that "functions in the hook"
aren't like "functions in a list".  The symbol `t` may be in "in the
hook".

But it's better for practical purposed, I admit.  Because "result" is
often conflated with "return value", and that's only _one_ of the ways
the functions can produce doc strings, I'd modify that to:

   `eldoc-documentation-compose': calls all the functions in the hook,
   and displays the doc strings that they produce...
>
> This doc string doesn't explain the use of the timer, it explains the
> reason for its existence.  It should also describe the use:
>
>> +(defvar eldoc--enthusiasm-curbing-timer nil
>> +  "Timer used by `eldoc-documentation-enthusiast' to avoid
>blinking.")

OK.

> Last, but not least: the "async" part of the branch's name hints on
> some advanced and extremely useful functionality that these changes
> are supposed to allow, but I see no mention of that in NEWS and in the
> manual bits.  What did I miss?

Not much, we talked about this.  Will change NEWS according to what we
discussed.

João





reply via email to

[Prev in Thread] Current Thread [Next in Thread]