bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Thibault Polge]

[-- Attachment #1: Type: text/plain, Size: 155 bytes --]

The attached patch links the docstring of `compilation-finish-functions`
to the info manual page documenting the string argument those functions
receive.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Clarify-documentation-of-compilation-finish-function.patch --]
[-- Type: text/x-patch, Size: 1079 bytes --]

From ff48c17796acb3d6a77c893bd2b58bff62d43b03 Mon Sep 17 00:00:00 2001
From: Thibault Polge <thibault@thb.lt>
Date: Sun, 21 Nov 2021 11:41:51 +0100
Subject: [PATCH] Clarify documentation of `compilation-finish-functions`

Link to reference on the possible values of the string argument.
---
 lisp/progmodes/compile.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index ac26f5e934..b71ee410bb 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -114,7 +114,10 @@ compilation-buffer-name-function
 (defvar compilation-finish-functions nil
   "Functions to call when a compilation process finishes.
 Each function is called with two arguments: the compilation buffer,
-and a string describing how the process finished.")
+and a string describing how the process finished.
+
+That string is the sentinel's event type string, see info
+node `(elisp) Sentinels' for more details.")
 
 (defvar compilation-in-progress nil
   "List of compilation processes now running.")
-- 
2.33.1


[-- Attachment #3: Type: text/plain, Size: 24 bytes --]


Best regards,
Thibault

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Eli Zaretskii]

> From: Thibault Polge <thibault@thb.lt>
> Date: Sun, 21 Nov 2021 11:48:00 +0100
> 
>  (defvar compilation-finish-functions nil
>    "Functions to call when a compilation process finishes.
>  Each function is called with two arguments: the compilation buffer,
> -and a string describing how the process finished.")
> +and a string describing how the process finished.
> +
> +That string is the sentinel's event type string, see info
> +node `(elisp) Sentinels' for more details.")


But this is not entirely accurate, is it?  The code actually does

           (let ((out-string (format ":%s [%s]" process-status (cdr status)))
                 (msg (format "%s %s" mode-name
                              (replace-regexp-in-string "\n?$" ""
                                                        (car status)))))
    [...]
    (run-hook-with-args 'compilation-finish-functions cur-buffer msg)))

So 'msg' is not just the process status , AFAIU.

Thanks.

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Thibault Polge]

> But this is not entirely accurate, is it?

I've done a quick test, and this seems accurate.  As I understand it,
the code you quoted:

>            (let ((out-string (format ":%s [%s]" process-status (cdr status)))
>                  (msg (format "%s %s" mode-name
>                               (replace-regexp-in-string "\n?$" ""
>                                                         (car status)))))

comes from a let block that happens in the context of:

(setq mode-line-process [...]

That is used to, IIUC, decorate the mode line with info about the
compilation result.  This setq, and the let it wraps, ends just
before:

    (force-mode-line-update)
    [...]
    (run-hook-with-args 'compilation-finish-functions cur-buffer msg)))

So the msg that compilation-handle-exit receives is passed, unmodified,
to all 'compilation-finish-functions`.

Best regards,
Thibault

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Eli Zaretskii]

> From: Thibault Polge <thibault@thb.lt>
> Cc: 52018@debbugs.gnu.org
> Date: Sun, 21 Nov 2021 20:08:56 +0100
> 
>     (force-mode-line-update)
>     [...]
>     (run-hook-with-args 'compilation-finish-functions cur-buffer msg)))
> 
> So the msg that compilation-handle-exit receives is passed, unmodified,
> to all 'compilation-finish-functions`.

Maybe so, but I'm looking at the calls of compilation-handle-exit, and
the callers do all kinds of transformations with the original status,
so I think documenting what happens to be more-or-less correct now is
fragile.

Why do you think this addition is of such great importance to have it
spelled out in the doc string?

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Thibault Polge]

> Maybe so, but I'm looking at the calls of compilation-handle-exit, and
> the callers do all kinds of transformations with the original status,
> so I think documenting what happens to be more-or-less correct now is
> fragile.

I assumed this interface was reasonably stable, I may be wrong.

> Why do you think this addition is of such great importance to have it
> spelled out in the doc string?

All the docstring currently states about that argument is that it's “a
string describing how the process finished”, which requires users to
determine its possible values through trial and error, or by digging
into the code.  I assumed it would be nicer (but not of “great
importance”) to just document it a bit further.

Best regards,

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Eli Zaretskii]

> From: Thibault Polge <thibault@thb.lt>
> Cc: 52018@debbugs.gnu.org
> Date: Sun, 21 Nov 2021 20:44:46 +0100
> 
> > Why do you think this addition is of such great importance to have it
> > spelled out in the doc string?
> 
> All the docstring currently states about that argument is that it's “a
> string describing how the process finished”, which requires users to
> determine its possible values through trial and error, or by digging
> into the code.

I'm asking why do you think they should care.  If they want the
process exit status, they can have it, can't they?

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Thibault Polge]

> I'm asking why do you think they should care.  If they want the
> process exit status, they can have it, can't they?

I honestly don't know how to do that, but if they can, I guess all is well.

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Eli Zaretskii]

> From: Thibault Polge <thibault@thb.lt>
> Cc: 52018@debbugs.gnu.org
> Date: Sun, 21 Nov 2021 21:00:20 +0100
> 
> > I'm asking why do you think they should care.  If they want the
> > process exit status, they can have it, can't they?
> 
> I honestly don't know how to do that, but if they can, I guess all is well.

The function is called with the compilation buffer, right?  And the
compilation process runs in that buffer, right?  So the function can
call get-buffer-process, and then access the exit status via
process-exit-status.

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Thibault Polge]

> The function is called with the compilation buffer, right?  And the
> compilation process runs in that buffer, right?  So the function can
> call get-buffer-process, and then access the exit status via
> process-exit-status.

I actually tried that, but the docstring for get-buffer-process says the
process must be live.  The following attempt fails:

(add-hook 'compilation-finish-functions
          (defun my-compilation-finish-function (buffer _)
            (let ((process (get-buffer-process buffer)))
              (message "Is there a buffer? %s" (bufferp buffer))
              (message "Is there a process? %s" (not (null process)))
              (message "Its status is: %s" (process-exit-status process)))))

Fails with:

Is there a buffer? t
Is there a process? nil
error in process sentinel: message: Wrong type argument: processp, nil
error in process sentinel: Wrong type argument: processp, nil

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Eli Zaretskii]

> From: Thibault Polge <thibault@thb.lt>
> Cc: 52018@debbugs.gnu.org
> Date: Sun, 21 Nov 2021 21:20:28 +0100
> 
> > The function is called with the compilation buffer, right?  And the
> > compilation process runs in that buffer, right?  So the function can
> > call get-buffer-process, and then access the exit status via
> > process-exit-status.
> 
> I actually tried that, but the docstring for get-buffer-process says the
> process must be live.  The following attempt fails:

OK, then the info is also on the mode line and can be taken from
there.

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Thibault Polge]

> OK, then the info is also on the mode line and can be taken from
> there.

I'm not sure I follow you. Are you suggesting functions in
`compilation-finish-functions` should scrape the mode line to get the
compilation process' exit status, instead of just using the string
argument they receive?

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Eli Zaretskii]

> From: Thibault Polge <thibault@thb.lt>
> Cc: 52018@debbugs.gnu.org
> Date: Mon, 22 Nov 2021 10:12:46 +0100
> 
> > OK, then the info is also on the mode line and can be taken from
> > there.
> 
> I'm not sure I follow you. Are you suggesting functions in
> `compilation-finish-functions` should scrape the mode line to get the
> compilation process' exit status, instead of just using the string
> argument they receive?

No need to scrape the mode line: the info is readily available in
mode-line-process.

If the purpose is to provide the exit status to the function, we need
to take care to pass it without any massaging.  We don't make a point
of doing that now, so I'd prefer not to document this in such certain
terms, as people will then complain that it isn't 100% accurate.

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Thibault Polge]

> If the purpose is to provide the exit status to the function, we need
> to take care to pass it without any massaging.  We don't make a point
> of doing that now, so I'd prefer not to document this in such certain
> terms, as people will then complain that it isn't 100% accurate.

Thanks for the clarification. This makes sense, I'm just not sure how to
act on this.  Would you rather drop this change or modify it to use more
cautious language?  

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Eli Zaretskii]

> From: Thibault Polge <thibault@thb.lt>
> Cc: 52018@debbugs.gnu.org
> Date: Tue, 23 Nov 2021 10:26:50 +0100
> 
> > If the purpose is to provide the exit status to the function, we need
> > to take care to pass it without any massaging.  We don't make a point
> > of doing that now, so I'd prefer not to document this in such certain
> > terms, as people will then complain that it isn't 100% accurate.
> 
> Thanks for the clarification. This makes sense, I'm just not sure how to
> act on this.  Would you rather drop this change or modify it to use more
> cautious language?  

Not sure, actually.  Lars, WDYT?

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > If the purpose is to provide the exit status to the function, we need
>> > to take care to pass it without any massaging.  We don't make a point
>> > of doing that now, so I'd prefer not to document this in such certain
>> > terms, as people will then complain that it isn't 100% accurate.
>> 
>> Thanks for the clarification. This makes sense, I'm just not sure how to
>> act on this.  Would you rather drop this change or modify it to use more
>> cautious language?  
>
> Not sure, actually.  Lars, WDYT?

The string that's passed in is a pretty free-form string, so I'm not
sure what we could say about it in the manual that'd be meaningful.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Thibault Polge <thibault@thb.lt>,  52018@debbugs.gnu.org
> Date: Wed, 24 Nov 2021 08:02:52 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Thanks for the clarification. This makes sense, I'm just not sure how to
> >> act on this.  Would you rather drop this change or modify it to use more
> >> cautious language?  
> >
> > Not sure, actually.  Lars, WDYT?
> 
> The string that's passed in is a pretty free-form string, so I'm not
> sure what we could say about it in the manual that'd be meaningful.

So I guess we want to drop this patch, since the existing doc string
already says vaguely enough that the string describes how the process
finished.

bug#52018: 28.0.60; Improve documentation for compilation-finish-functions

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> So I guess we want to drop this patch, since the existing doc string
> already says vaguely enough that the string describes how the process
> finished.

Yup.  I'm closing this issue, then.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no