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:

> J.P. wrote:
>
>> Make erc-cmd-AMSG session-local, add /GMSG /AME /GME
>>
>> * lisp/erc/erc.el (erc-cmd-AMSG): Make good on behavior described in
>> the doc string by limiting damage to the current connection.
>> (erc-cmd-GMSG, erc-cmd-GME, erc-cmd-AME): New functions, all IRC
>> "slash commands".  (Bug#68401)
>
> Okay, I'll use that instead. But let's agree on the
> source first.
>
>> I question the wisdom of having new slash commands serve
>> double duty as interactive Emacs commands (at least those
>> handling chat input). This reservation has nothing to do
>> with M-x erc-cmd-FOO <RET> being less faithful (or whatever)
>> to the traditional IRC experience than /FOO <RET>. Rather,
>> it stems from a need to prioritize consistent feedback and
>> promote maintainability by only having a single path for
>> chat input to reach the server (except under special
>> circumstances).
>
> I made them interactive as `erc-cmd-AMSG' is interactive, but
> let's remove it from the other three then.
>
> [ As a side note, Emacs has a problem with different
>   interfaces doing too much and influencing the behavior of
>   their functions. Interfaces should just be different ways of
>   setting the formal parameters, after that the exact same
>   thing should happen for the same data. ]
>
>>> (setq line (erc-trim-string line))
>>
>> It might be nice to remove at most one space, for cases
>> where a user wants to send preformatted text. OTOH, normal
>> /MSG doesn't do this, so perhaps we shouldn't here either.
>
> Again, this is in the original `erc-cmd-AMSG'. I have no
> opinion, so you can decide it.
>
> "At most one space", what space should that be? Leading or
> trailing?

Leading. See the test for `erc-extract-command-from-line' to understand
the behavior of `do-not-parse-args', which determines LINE. Actually, if
we're doing away with `commandp', there should be no reason for "at most
one," only "exactly one" (IIRC).

> This is nothing `erc-trim-string' can do BTW. But we
> can of course still remove whatever spaces we like.

I wasn't implying you ought to change `erc-trim-string' but rather that
you can replace its call with an expression to remove a leading space.

>
>>>    (erc-with-all-buffers-of-server nil
>>> -    (lambda ()
>>> -      (erc-channel-p (erc-default-target)))
>>> +    (lambda () (erc-channel-p (erc-default-target)))
>>> +    (erc-send-message line)))
>>
>> Without first checking for connectivity, we run into another
>> situation in which messages may be inserted but not sent,
>> similar to the bit about commands being potentially
>> "misleading," above. The most obvious way to solve this is
>> to check for "physical" connectivity with something like:
>>
>>   (erc-with-all-buffers-of-server nil #'erc-server-process-alive
>>     (when (and erc--target (erc--current-buffer-joined-p))
>>       (erc-send-message line))))
>>
>> Alternatively, you can check for "logical" connectivity,
>> which is probably more in keeping with traditional design
>> principles:
>>
>>   (erc-with-all-buffers-of-server nil nil
>>     (when (and erc-server-connected erc--target (erc--current-buffer-joined-p))
>>       (erc-send-message line))))
>>
>> One minor downside of this second method is that IRC
>> adjacent protocols and aberrant proxy servers that happen to
>> skip 376/422 and also provide some (possibly &local)
>> "control channel" won't be detected. (BTW, you won't be
>> needing the `erc--target' in either example if you rebase
>> atop the latest master.)
>
> Okay, but instead of having these checks embedded and hopefully
> correctly repeated four times, shouldn't we have two functions, say
> "erc-connected-physical-p" and "erc-connected-logical-p" and call
> either of those (or both) from the functions?

If you want to factor out a common helper function, fine by me. AFAICT
such a thing would need to include `erc-with-all-buffers-of-server' to
be effective unless the predicates you've named alone result in
meaningful code reuse. (Not sure how an `erc-connected-physical-p' would
be any different than the existing 'erc-server-process-alive', though I
suppose an `erc-connected-logical-p' could be useful if it just returns
`erc-server-connected'.)

>>> +(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 (erc-trim-string line))
>>> +  (erc-with-all-buffers-of-server erc-server-process
>>> +    (lambda () (erc-channel-p (erc-default-target)))
>>
>>          ^ Indentation. This macro is declared "indent 2"
>
> Okay, fixed that.
>
>> rebase
>
> How do you do that, just 'git pull'?

Assuming you have your work on "my-branch"

  $ git checkout master
  $ git pull
  $ git rebase master my-branch.

If you applied the unit-test patch atop your commit, you won't be able
to "git commit --amend" your previous changes. See the "-i,
--interactive" option for git-rebase(1), then maybe rearrange things so
your patch comes *after* the test. You can always "git rebase --abort"
if you mess up. And there's always #git on Libera.