Seeking Advice about refactoring and advice snippet

Seeking Advice about refactoring and advice snippet

[Filipe Silva]

Dear good people of the emacs help list,

I have a working snippet that advices both kill-buffer and kill-this-buffer
to not kill the *scratch* buffer:

      (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
        (let ((buffer-to-kill (buffer-name (current-buffer))))
          (if (equal buffer-to-kill "*scratch*")
              (message "DENIED! don't kill my precious *scratch*!!")
            (apply buffer-assassin arguments))))
      (defun ninrod/scratch-protection (buffer-assassin &rest arguments)
        (let ((buffer-to-kill (car arguments)))
          (if (equal buffer-to-kill "*scratch*")
              (message "DENIED! don't kill my precious *scratch*!!")
            (apply buffer-assassin arguments))))
      (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
      (advice-add #'kill-buffer :around #'ninrod/scratch-protection)

The problem is that these lines:

      (message "DENIED! don't kill my precious *scratch*!!")
     (apply buffer-assassin arguments))))

Are repeated in both functions, so I thought that I could apply the DRY
principle and refactor the snippet to this:

    (defun ninrod--protection (buffer-assassin buffer-to-kill &rest
arguments)
      (if (equal buffer-to-kill "*scratch*")
          (message "DENIED! don't kill my precious *scratch*!!")
        (apply buffer-assassin arguments)))
    (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
      (let ((buffer-to-kill (buffer-name (current-buffer))))
        (ninrod--protection 'buffer-assassin buffer-to-kill arguments)))
    (defun ninrod/scratch-protection (buffer-assassin &rest arguments)
      (let ((buffer-to-kill (car arguments)))
        (ninrod--protection 'buffer-assassin buffer-to-kill arguments)))
    (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
    (advice-add #'kill-buffer :around #'ninrod/scratch-protection)

This causes all hell to break loose. Now I can't even close emacs, because
apparently emacs tries to kill all buffers
and as I've just tampered with the kill buffer functions, well, it's bad.
Very bad.

I know I mean well, but I'm must be doing something very stupid. For
starters, I don't know if I can really pass around
functions as parameters? So it could be that?

How would you refactor that snippet to apply the dry principle?

thanks in advance,

Filipe.

Re: Seeking Advice about refactoring and advice snippet

[Yuri Khan]

On Fri, Feb 10, 2017 at 6:43 PM, Filipe Silva <filipe.silva@gmail.com> wrote:

> The problem is that these lines:
>
>       (message "DENIED! don't kill my precious *scratch*!!")
>      (apply buffer-assassin arguments))))
>
> Are repeated in both functions, so I thought that I could apply the DRY
> principle

I cannot say anything useful regarding your approach, but I can’t help
noticing that you are trying to reduce duplication of two lines in two
functions. This is not likely to help maintainability significantly.

In Extreme Programming circles, there is a principle called Three
Strikes And You Refactor, which says that you only have enough data
for a meaningful refactoring when you see three instances of the same
pattern.

Until you encounter that third instance, you could follow the WET
principle instead: Write Everything Twice :)

Re: Seeking Advice about refactoring and advice snippet

[Narendra Joshi]

Filipe Silva <filipe.silva@gmail.com> writes:

> Dear good people of the emacs help list,
>
> I have a working snippet that advices both kill-buffer and kill-this-buffer
> to not kill the *scratch* buffer:
>
>       (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
>         (let ((buffer-to-kill (buffer-name (current-buffer))))
>           (if (equal buffer-to-kill "*scratch*")
>               (message "DENIED! don't kill my precious *scratch*!!")
>             (apply buffer-assassin arguments))))
>       (defun ninrod/scratch-protection (buffer-assassin &rest arguments)
>         (let ((buffer-to-kill (car arguments)))
>           (if (equal buffer-to-kill "*scratch*")
>               (message "DENIED! don't kill my precious *scratch*!!")
>             (apply buffer-assassin arguments))))
>       (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
>       (advice-add #'kill-buffer :around #'ninrod/scratch-protection)
>
> The problem is that these lines:
>
>       (message "DENIED! don't kill my precious *scratch*!!")
>      (apply buffer-assassin arguments))))
>
> Are repeated in both functions, so I thought that I could apply the DRY
> principle and refactor the snippet to this:
>
>     (defun ninrod--protection (buffer-assassin buffer-to-kill &rest
> arguments)
>       (if (equal buffer-to-kill "*scratch*")
>           (message "DENIED! don't kill my precious *scratch*!!")
>         (apply buffer-assassin arguments)))
>     (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
>       (let ((buffer-to-kill (buffer-name (current-buffer))))
>         (ninrod--protection 'buffer-assassin buffer-to-kill arguments)))
>     (defun ninrod/scratch-protection (buffer-assassin &rest arguments)
>       (let ((buffer-to-kill (car arguments)))
>         (ninrod--protection 'buffer-assassin buffer-to-kill arguments)))
>     (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
>     (advice-add #'kill-buffer :around #'ninrod/scratch-protection)
>
> This causes all hell to break loose. Now I can't even close emacs, because
> apparently emacs tries to kill all buffers
> and as I've just tampered with the kill buffer functions, well, it's bad.
> Very bad.

Why don't you call `called-interactively-p' to decide whether you would
want to kill *scratch* or not? No other function should kill *scratch*
anyways except `save-buffers-kill-terminal'. 

> I know I mean well, but I'm must be doing something very stupid. For
> starters, I don't know if I can really pass around
> functions as parameters? So it could be that?
>
> How would you refactor that snippet to apply the dry principle?
>
> thanks in advance,
>
> Filipe.

-- 
Narendra Joshi

Re: Seeking Advice about refactoring and advice snippet

[Chunyang Xu]

Hi,

It should be easier to use 'kill-buffer-query-functions'

  (defun dont-kill-scratch ()
    (if ((equal (buffer-name) "*scratch*"))
        (progn (message "DENIED! don't kill my precious *scratch*!!")
               nil)
      t))
  
  (add-hook 'kill-buffer-query-functions #'dont-kill-scratch)

Filipe Silva writes:

> Dear good people of the emacs help list,
>
> I have a working snippet that advices both kill-buffer and kill-this-buffer
> to not kill the *scratch* buffer:
>
>       (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
>         (let ((buffer-to-kill (buffer-name (current-buffer))))
>           (if (equal buffer-to-kill "*scratch*")
>               (message "DENIED! don't kill my precious *scratch*!!")
>             (apply buffer-assassin arguments))))
>       (defun ninrod/scratch-protection (buffer-assassin &rest arguments)
>         (let ((buffer-to-kill (car arguments)))
>           (if (equal buffer-to-kill "*scratch*")
>               (message "DENIED! don't kill my precious *scratch*!!")
>             (apply buffer-assassin arguments))))
>       (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
>       (advice-add #'kill-buffer :around #'ninrod/scratch-protection)

Considering 'kill-this-buffer' calls 'kill-buffer', advising
'kill-buffer' alone is enough.

> The problem is that these lines:
>
>       (message "DENIED! don't kill my precious *scratch*!!")
>      (apply buffer-assassin arguments))))
>
> Are repeated in both functions, so I thought that I could apply the DRY
> principle and refactor the snippet to this:
>
>     (defun ninrod--protection (buffer-assassin buffer-to-kill &rest
> arguments)
>       (if (equal buffer-to-kill "*scratch*")
>           (message "DENIED! don't kill my precious *scratch*!!")
>         (apply buffer-assassin arguments)))
>     (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
>       (let ((buffer-to-kill (buffer-name (current-buffer))))
>         (ninrod--protection 'buffer-assassin buffer-to-kill arguments)))
>     (defun ninrod/scratch-protection (buffer-assassin &rest arguments)
>       (let ((buffer-to-kill (car arguments)))
>         (ninrod--protection 'buffer-assassin buffer-to-kill arguments)))
>     (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
>     (advice-add #'kill-buffer :around #'ninrod/scratch-protection)

You should not quote 'buffer-assassin'. And when you pass '&rest
arguments' to 'ninrod/scratch-protection', you should use
'apply'. Something like the following works from here.

  (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
    (let ((buffer-to-kill (buffer-name (current-buffer))))
      (apply #'ninrod--protection buffer-assassin buffer-to-kill arguments)))
  
  (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)

> This causes all hell to break loose. Now I can't even close emacs, because
> apparently emacs tries to kill all buffers
> and as I've just tampered with the kill buffer functions, well, it's bad.
> Very bad.
>
> I know I mean well, but I'm must be doing something very stupid. For
> starters, I don't know if I can really pass around
> functions as parameters? So it could be that?
>
> How would you refactor that snippet to apply the dry principle?
>
> thanks in advance,
>
> Filipe.

Re: Seeking Advice about refactoring and advice snippet

[Filipe Silva]

Hi Yuri, this WET principle is very insteresting and I should apply it
more. Thanks for sharing it!

In the interest of learning elisp properly though, I'd like to be able to
perfom this simple refactoring, because that would
improve my knowledge of elisp. This is a very simple operation that I
should be capable of performing.

[]s

Filipe

On Fri, Feb 10, 2017 at 11:08 AM, Yuri Khan <yuri.v.khan@gmail.com> wrote:

> On Fri, Feb 10, 2017 at 6:43 PM, Filipe Silva <filipe.silva@gmail.com>
> wrote:
>
> > The problem is that these lines:
> >
> >       (message "DENIED! don't kill my precious *scratch*!!")
> >      (apply buffer-assassin arguments))))
> >
> > Are repeated in both functions, so I thought that I could apply the DRY
> > principle
>
> I cannot say anything useful regarding your approach, but I can’t help
> noticing that you are trying to reduce duplication of two lines in two
> functions. This is not likely to help maintainability significantly.
>
> In Extreme Programming circles, there is a principle called Three
> Strikes And You Refactor, which says that you only have enough data
> for a meaningful refactoring when you see three instances of the same
> pattern.
>
> Until you encounter that third instance, you could follow the WET
> principle instead: Write Everything Twice :)
>

Re: Seeking Advice about refactoring and advice snippet

[Filipe Silva]

Chunyang, thanks for the alternative approach!

I'd really like to know how to perform this simple refactoring though, just
to improve my knowledge of elisp a little bit.

[]s

Filipe

On Fri, Feb 10, 2017 at 12:17 PM, Chunyang Xu <mail@xuchunyang.me> wrote:

> Hi,
>
> It should be easier to use 'kill-buffer-query-functions'
>
>   (defun dont-kill-scratch ()
>     (if ((equal (buffer-name) "*scratch*"))
>         (progn (message "DENIED! don't kill my precious *scratch*!!")
>                nil)
>       t))
>
>   (add-hook 'kill-buffer-query-functions #'dont-kill-scratch)
>
> Filipe Silva writes:
>
> > Dear good people of the emacs help list,
> >
> > I have a working snippet that advices both kill-buffer and
> kill-this-buffer
> > to not kill the *scratch* buffer:
> >
> >       (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
> >         (let ((buffer-to-kill (buffer-name (current-buffer))))
> >           (if (equal buffer-to-kill "*scratch*")
> >               (message "DENIED! don't kill my precious *scratch*!!")
> >             (apply buffer-assassin arguments))))
> >       (defun ninrod/scratch-protection (buffer-assassin &rest arguments)
> >         (let ((buffer-to-kill (car arguments)))
> >           (if (equal buffer-to-kill "*scratch*")
> >               (message "DENIED! don't kill my precious *scratch*!!")
> >             (apply buffer-assassin arguments))))
> >       (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
> >       (advice-add #'kill-buffer :around #'ninrod/scratch-protection)
>
> Considering 'kill-this-buffer' calls 'kill-buffer', advising
> 'kill-buffer' alone is enough.
>
> > The problem is that these lines:
> >
> >       (message "DENIED! don't kill my precious *scratch*!!")
> >      (apply buffer-assassin arguments))))
> >
> > Are repeated in both functions, so I thought that I could apply the DRY
> > principle and refactor the snippet to this:
> >
> >     (defun ninrod--protection (buffer-assassin buffer-to-kill &rest
> > arguments)
> >       (if (equal buffer-to-kill "*scratch*")
> >           (message "DENIED! don't kill my precious *scratch*!!")
> >         (apply buffer-assassin arguments)))
> >     (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
> >       (let ((buffer-to-kill (buffer-name (current-buffer))))
> >         (ninrod--protection 'buffer-assassin buffer-to-kill arguments)))
> >     (defun ninrod/scratch-protection (buffer-assassin &rest arguments)
> >       (let ((buffer-to-kill (car arguments)))
> >         (ninrod--protection 'buffer-assassin buffer-to-kill arguments)))
> >     (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
> >     (advice-add #'kill-buffer :around #'ninrod/scratch-protection)
>
> You should not quote 'buffer-assassin'. And when you pass '&rest
> arguments' to 'ninrod/scratch-protection', you should use
> 'apply'. Something like the following works from here.
>
>   (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
>     (let ((buffer-to-kill (buffer-name (current-buffer))))
>       (apply #'ninrod--protection buffer-assassin buffer-to-kill
> arguments)))
>
>   (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
>
> > This causes all hell to break loose. Now I can't even close emacs,
> because
> > apparently emacs tries to kill all buffers
> > and as I've just tampered with the kill buffer functions, well, it's bad.
> > Very bad.
> >
> > I know I mean well, but I'm must be doing something very stupid. For
> > starters, I don't know if I can really pass around
> > functions as parameters? So it could be that?
> >
> > How would you refactor that snippet to apply the dry principle?
> >
> > thanks in advance,
> >
> > Filipe.
>
>

Re: Seeking Advice about refactoring and advice snippet

[Filipe Silva]

Well, someone at emacs.stackexchange pointed out the the &rest keyword used
in my ninrod--protection function was to blame. That fixed the problem.

thanks!

On Fri, Feb 10, 2017 at 9:43 AM, Filipe Silva <filipe.silva@gmail.com>
wrote:

> Dear good people of the emacs help list,
>
> I have a working snippet that advices both kill-buffer and
> kill-this-buffer to not kill the *scratch* buffer:
>
>       (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
>         (let ((buffer-to-kill (buffer-name (current-buffer))))
>           (if (equal buffer-to-kill "*scratch*")
>               (message "DENIED! don't kill my precious *scratch*!!")
>             (apply buffer-assassin arguments))))
>       (defun ninrod/scratch-protection (buffer-assassin &rest arguments)
>         (let ((buffer-to-kill (car arguments)))
>           (if (equal buffer-to-kill "*scratch*")
>               (message "DENIED! don't kill my precious *scratch*!!")
>             (apply buffer-assassin arguments))))
>       (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
>       (advice-add #'kill-buffer :around #'ninrod/scratch-protection)
>
> The problem is that these lines:
>
>       (message "DENIED! don't kill my precious *scratch*!!")
>      (apply buffer-assassin arguments))))
>
> Are repeated in both functions, so I thought that I could apply the DRY
> principle and refactor the snippet to this:
>
>     (defun ninrod--protection (buffer-assassin buffer-to-kill &rest
> arguments)
>       (if (equal buffer-to-kill "*scratch*")
>           (message "DENIED! don't kill my precious *scratch*!!")
>         (apply buffer-assassin arguments)))
>     (defun ninrod/scratch-bodyguard (buffer-assassin &rest arguments)
>       (let ((buffer-to-kill (buffer-name (current-buffer))))
>         (ninrod--protection 'buffer-assassin buffer-to-kill arguments)))
>     (defun ninrod/scratch-protection (buffer-assassin &rest arguments)
>       (let ((buffer-to-kill (car arguments)))
>         (ninrod--protection 'buffer-assassin buffer-to-kill arguments)))
>     (advice-add #'kill-this-buffer :around #'ninrod/scratch-bodyguard)
>     (advice-add #'kill-buffer :around #'ninrod/scratch-protection)
>
> This causes all hell to break loose. Now I can't even close emacs, because
> apparently emacs tries to kill all buffers
> and as I've just tampered with the kill buffer functions, well, it's bad.
> Very bad.
>
> I know I mean well, but I'm must be doing something very stupid. For
> starters, I don't know if I can really pass around
> functions as parameters? So it could be that?
>
> How would you refactor that snippet to apply the dry principle?
>
> thanks in advance,
>
> Filipe.
>
>

Re: Seeking Advice about refactoring and advice snippet

[Stefan Monnier]

> How would you refactor that snippet to apply the dry principle?

Here's how I'd refactor it:

    (with-current-buffer (get-buffer "*scratch*")
      (add-hook 'kill-buffer-hook
                (lambda () (error "DENIED! don't kill my precious *scratch*!!"))
                nil t))

;-)


        Stefan


PS: Along the same lines: kill-this-buffer calls kill-buffer, so you
shouldn't need to advise kill-this-buffer.

Re: Seeking Advice about refactoring and advice snippet

[Filipe Silva]

Stefan I noticed that your hack works even with M-x extended-buffer-menu,
while my advices don't.

I don't understand fully what that snippet does but that's some powerful
stuff, thank you!

On Fri, Feb 10, 2017 at 5:55 PM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > How would you refactor that snippet to apply the dry principle?
>
> Here's how I'd refactor it:
>
>     (with-current-buffer (get-buffer "*scratch*")
>       (add-hook 'kill-buffer-hook
>                 (lambda () (error "DENIED! don't kill my precious
> *scratch*!!"))
>                 nil t))
>
> ;-)
>
>
>         Stefan
>
>
> PS: Along the same lines: kill-this-buffer calls kill-buffer, so you
> shouldn't need to advise kill-this-buffer.
>
>
>

Re: Seeking Advice about refactoring and advice snippet

[Stefan Monnier]

> Stefan I noticed that your hack works even with M-x extended-buffer-menu,
> while my advices don't.

Actually, Chunyang Xu's solution is probably even better.
I had forgotten about kill-buffer-query-functions.


        Stefan