bug#42386: Acknowledgement ([PATCH] Handle symbols in project-kill-buffers-ignores)

[Dmitry Gutov <dgutov@yandex.ru>]

On 18.07.2020 15:48, Philip K. wrote:
> I like this idea a lot, the patch below should implement this +
> backwards compatibility code. Thought this might be getting too
> complicated, I also went ahead and added "and" and "or".

All right. That's a bit further than I expected, but the result is still 
fast in the default scenario, so why not. ;-)

I take it this approach behaved well enough in your testing?

Should we replace

   (derived-mode . special-mode)

with

   (and (derived-mode . special-mode)
        (not (major-mode . help-mode)))

?

Some other minor comments below.

> 0001-Replace-project-kill-buffers-ignores-with-.-kill-buf.patch
> 
>  From 6a9c268a340025bca428b5ec7c35229a29b4a95f Mon Sep 17 00:00:00 2001
> From: Philip K<philip@warpmail.net>
> Date: Thu, 16 Jul 2020 10:03:35 +0200
> Subject: [PATCH] Replace project-kill-buffers-ignores with
>   ...-kill-buffer-conditions

Full commit message, if you can.

> +- a symbol, denoting a buffer local variable, where the buffer
> +  is killed if it's value is non-nil. If the symbol also has a
> +  function slot, it will be interpreted as a function first.

This also introduces an ambiguity which I'd like to avoid. Let's just 
make it if a symbol is there, it must be a function (and we should 
silence its errors).

> +Buffers that match any of the conditions will not be killed."

Will be. I think.

> +(defcustom project-kill-buffers-ignores nil
> +  "Conditions for buffers `project-kill-buffers' should not kill."
> +  :type '(repeat choice regexp function)
> +  :set (lambda (var val)
> +         (add-to-list 'project-kill-buffer-conditions
> +                      (cons 'not val))
> +         (custom-set-default var val))
> +  :version "28.1"
> +  :group 'project
> +  :package-version '(project . "0.6.0")))

Nice thought, but I think we're allowed to simply do away with this 
variable.

At least I have been informed that as long as the package version 
haven't been in a "proper" Emacs release, its contents don't have the 
same backward compatibility promise.

> +(defun project--kill-buffer-check (buf &optional conds)
> +  "Throw"

Just so you don't forget to update or delete this docstring.

> +  (unless conds
> +    (setq conds project-kill-buffer-conditions))

I think we can make the CONDS argument required and pass in this value 
from project-kill-buffers.

> +  (catch (if (eq project-kill-buffer-conditions conds)
> +             'kill 'other)

Do we really need this condition?

> +    (dolist (c conds)
> +      (when (cond
> +             ((stringp c)
> +              (string-match-p c (buffer-name buf)))
> +             ((and (functionp c)

Let's just make this (symbolp c).

> +                   (ignore-errors (funcall c buf))))

and remove the 'ignore-errors' form. It's better to inform the user 
right away that their predicate is broken.

> +             ((and (symbolp c) (boundp c))
> +              (buffer-local-value c buf))

And remove this case.

> +             ((eq (car-safe c) 'major-mode)
> +              (eq (buffer-local-value 'major-mode buf)
> +                  (cdr c)))
> +             ((eq (car-safe c) 'derived-mode)
> +              (provided-mode-derived-p
> +               (buffer-local-value 'major-mode buf)
> +               (cdr c)))
> +             ((eq (car-safe c) 'not)
> +              (not (project--kill-buffer-check buf (cdr c))))
> +             ((eq (car-safe c) 'and)
> +              (seq-every-p
> +               (apply-partially #'project--kill-buffer-check
> +                                buf)
> +               (cdr c)))
> +             ((eq (car-safe c) 'or)
> +              (seq-some
> +               (apply-partially #'project--kill-buffer-check
> +                                buf)

I think we can simply recurse in this case.

Thanks!