GNU bug report logs - #75956
Various improvements to sieve.el

Previous Next

Full log

View this message in rfc822 format

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Björn Bidar <bjorn.bidar <at> thaodan.de>,  Eli Zaretskii <eliz <at> gnu.org>
Cc: 75956 <at> debbugs.gnu.org
Subject: bug#75956: Various improvements to sieve.el
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" <bug-gnu-emacs <at> gnu.org> writes:

> Eli Zaretskii <eliz <at> gnu.org> 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" <bug-gnu-emacs <at> gnu.org>
>>>
>>> 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?= <bjorn.bidar <at> thaodan.de>
> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.