Subject: bug#75956: Various improvements to sieve.el

From: Stefan Kangas <[email-masked]>
Date: Mon, 10 Feb 2025 23:59:57 -0800

Björn Bidar via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <[email-masked]> writes:

> Eli Zaretskii <[email-masked]> writes:
>
>>> Date: Fri, 31 Jan 2025 03:54:11 +0200
>>> From:  Björn Bidar via "Bug reports for GNU Emacs,
>>>  the Swiss army knife of text editors" <[email-masked]>
>>>
>>> I noticed it is not possible to specific the sieve-manage port when
>>> calling `sieve-manage`. While fixing that I noticed various other issues.
>>> - Missing documentation strings on many functions
>>> - There is function to refresh the buffer but it is not bound
>>>
>>> The attached patches try to fix these issues.
>>
>> Thanks, please see some comments below.

Some additional comments from me.

>>>  (defun sieve-manage (server &optional port)
>>> -  (interactive "sServer: ")
>>> +  "Open ManageSieve SERVER.
>>> +Optionally with PORT.
>>
>> This is too cryptic.  It should be something like
>>
>>   Optional argument PORT specifies the port to use; it defaults to
>>   `sieve-manage-default-port'.
>
> Isn't that kinda redundant? Especially saying port twice doesn't help to
> understand the purpose the variable is self explanatory.

It's not redundant, no.

> From 7b2d6c3c4bfaa0611c6a23b452c0b40831c41696 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Bj=C3=B6rn=20Bidar?= <[email-masked]>
> Date: Fri, 31 Jan 2025 02:34:19 +0200
> Subject: [PATCH 1/3] * lisp/net/sieve.el: Read port in sieve-manage when
>  interactive
>
> (sieve-manage): Optionally read port when called interactively.
> Add previously missing documentation string.

Please add the bug number here and to the other patches, like this:

    Add missing documentation string.  (Bug#75956)

> ---
>  lisp/net/sieve.el | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/net/sieve.el b/lisp/net/sieve.el
> index f9f036e1589..c67d8b34ce0 100644
> --- a/lisp/net/sieve.el
> +++ b/lisp/net/sieve.el
> @@ -338,7 +338,14 @@ sieve-refresh-scriptlist
>
>  ;;;###autoload
>  (defun sieve-manage (server &optional port)
> -  (interactive "sServer: ")
> +  "Open ManageSieve SERVER.
> +Optionally argument PORT specifies the port use;
> +otherwise it defaults to `sieve-manage-default-port'.
> +
> +When called interactively, prompt for SERVER:PORT
> +if :PORT is not specified fall back to `sieve-manage-default-port'."
> +  (interactive
> +   (split-string (read-string "Server: ") ":"))

How are users that see a prompt for "Server: " supposed to know that
they are to enter a part also?  What will happen if the user doesn't do
that?

I recommend prompting first for server, then for port.  Also, how about
providing some defaults (does "localhost" and some standard port make
sense)?

The rest of your patches LGTM.