Re: bug#68401: 30.0.50; ERC 5.6-git: `erc-cmd-GMSG', `erc-cmd-AMSG', `erc-cmd-GME', `erc-cmd-AME'. 2nd attempt

Emanuel Berg <[email protected]> writes:

> Tags: patch
>
> Okay, thanks to a lot of help at #erc and #git I think this is
> one full patch for a single commit reflecting the desired
> change to the source.
>
> Let me know if it is okay.
>
>> From b006669b078a4cf4aec2ea19de1a954d494ec19f Mon Sep 17 00:00:00 2001
> From: Emanuel Berg <[email protected]>
> Date: Tue, 23 Jan 2024 14:21:49 +0100
> Subject: [PATCH 211/211] Make erc-cmd-AMSG session local; add /GMSG, /AME and
>  /GME
>
> * lisp/erc/erc.el (erc-cmd-AMSG): Make it consistent with the doc
> string by only affecting the current connection.
> (erc-cmd-GMSG, erc-cmd-AME, erc-cmd-GME): new IRC slash commands
> (Bug#68401)

Looks like you've combined ("fixed up") your patch with the provided
test and data files [1]. That's fine, but its changes should be listed
in the commit message as well:

  * test/lisp/erc/erc-scenarios-misc-commands.el
  (erc-scenarios-misc-commands--AMSG-GMSG-AME-GME): New test.
  * test/lisp/erc/resources/commands/amsg-barnet.eld: New file.
  * test/lisp/erc/resources/commands/amsg-foonet.eld: New file.

In the future, maybe look into "squashing" to preserve messages. Also,
consider adding an entry to etc/ERC-NEWS if you think people can benefit
from these commands.

[1] https://lists.gnu.org/archive/html/emacs-erc/2024-01/msg00038.html

> ---
>  lisp/erc/erc.el                               | 38 +++++++--
>  test/lisp/erc/erc-scenarios-misc-commands.el  | 84 +++++++++++++++++++
>  .../erc/resources/commands/amsg-barnet.eld    | 52 ++++++++++++
>  .../erc/resources/commands/amsg-foonet.eld    | 52 ++++++++++++
>  4 files changed, 220 insertions(+), 6 deletions(-)
>  create mode 100644 test/lisp/erc/resources/commands/amsg-barnet.eld
>  create mode 100644 test/lisp/erc/resources/commands/amsg-foonet.eld
>
> diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
> index 08dfa4b8f1b..15734b1010d 100644
> --- a/lisp/erc/erc.el
> +++ b/lisp/erc/erc.el
> @@ -4047,16 +4047,42 @@ erc--split-string-shell-cmd
>  ;;                    Input commands handlers
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  
> -(defun erc-cmd-AMSG (line)
> -  "Send LINE to all channels of the current server that you are on."
> -  (interactive "sSend to all channels you're on: ")
> -  (setq line (erc-trim-string line))
> +(defun erc--connected-and-joined-p ()
> +  (and (erc--current-buffer-joined-p)
> +       erc-server-connected))
> +
> +(defun erc-cmd-GMSG (line)
> +  "Send LINE to all channels on all networks you are on."
> +  (setq line (string-remove-prefix " " line))
>    (erc-with-all-buffers-of-server nil
> -    (lambda ()
> -      (erc-channel-p (erc-default-target)))
> +      #'erc--connected-and-joined-p
> +    (erc-send-message line)))
> +(put 'erc-cmd-GMSG 'do-not-parse-args t)
> +
> +(defun erc-cmd-AMSG (line)
> +  "Send LINE to all channels of the current network.
> +Interactively, prompt for the line of text to send."
> +  (interactive "sSend to all channels on this network: ")
> +  (setq line (string-remove-prefix " " line))
> +  (erc-with-all-buffers-of-server erc-server-process
> +      #'erc--connected-and-joined-p
>      (erc-send-message line)))
>  (put 'erc-cmd-AMSG 'do-not-parse-args t)
>  
> +(defun erc-cmd-GME (line)
> +  "Send LINE as an action to all channels on all networks you are on."
> +  (erc-with-all-buffers-of-server nil
> +      (erc--connected-and-joined-p)
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Seems one of these things is not like the others. Guessing that's
unintentional.

What's not great is that the test still passes in spite of this. It
seems /GME is the only variant not covered, which I guess is my fault.
Perhaps you should improve the test so it fails with the current patch
applied and passes once it's fixed.

> +    (erc-cmd-ME line)))
> +(put 'erc-cmd-GME 'do-not-parse-args t)
> +
> +(defun erc-cmd-AME (line)
> +  "Send LINE as an action to all channels on the current network."
> +  (erc-with-all-buffers-of-server erc-server-process
> +      #'erc--connected-and-joined-p
> +    (erc-cmd-ME line)))
> +(put 'erc-cmd-AME 'do-not-parse-args t)
> +
>  (defun erc-cmd-SAY (line)
>    "Send LINE to the current query or channel as a message, not a command.
>  
> diff --git a/test/lisp/erc/erc-scenarios-misc-commands.el b/test/lisp/erc/erc-scenarios-misc-commands.el
> index d6ed53b5358..c6bb610b9df 100644
> --- a/test/lisp/erc/erc-scenarios-misc-commands.el
> +++ b/test/lisp/erc/erc-scenarios-misc-commands.el
> @@ -123,4 +123,88 @@ erc-scenarios-misc-commands--VHOST
>          (should (string= (erc-server-user-host (erc-get-server-user "tester"))
>                           "some.host.test.cc"))))))
>  
> +;; This tests four related slash commands, /AMSG, /GMSG, /AME, /GME,
> +;; the latter three introduced by bug#68401.  It mainly asserts
> +;; correct routing behavior, especially not sending or inserting
> +;; messages in buffers belonging to disconnected sessions.  Left
> +;; unaddressed are interactions with the `command-indicator' module
> +;; (`erc-noncommands-list') and whatever future `echo-message'
> +;; implementation manifests out of bug#49860.
> +(ert-deftest erc-scenarios-misc-commands--AMSG-GMSG-AME-GME ()
> +  (erc-scenarios-common-with-cleanup
> +      ((erc-scenarios-common-dialog "commands")
> +       (erc-server-flood-penalty 0.1)
> +       (dumb-server-foonet (erc-d-run "localhost" t "srv-foonet" 'amsg-foonet))
> +       (dumb-server-barnet (erc-d-run "localhost" t "srv-barnet" 'amsg-barnet))
> +       (expect (erc-d-t-make-expecter)))
[...]

Is this FIXME comment [2] regarding your paperwork accurate?

[2] https://git.savannah.gnu.org/cgit/emacs/elpa.git/tree/elpa-packages?id=e5075ab1#n805