Re: bug#56340: 29.0.50; [PATCH] Don't ask people to order requires.

[J.P.]

Hi Dick, Lars, people,

Lars Ingebrigtsen <larsi@gnus.org> writes:

> dick.r.chiang@gmail.com writes:
>
>> Asking people to order require's is about as effective
>> as asking kids to keep off the grass.
>
> It looks like this patch is rearranging some dependencies to make erc
> load in a more logical way?
>
> I haven't tried the patch, but skimming it, it seems to make some sense.
> I've added the erc maintainers to the CCs.

This bug seems to be in answer to #54825, which, Dick, I guess you found
wanting enough to serve as your next meal (and its author simple enough
to adopt as your next mark). Perhaps all is right with the world.

Regardless of whatever ancillary motivations might have prompted that
perceived half measure from me [1], given the direction I'd like to see
ERC move in, a little turbulence in this arena is likely inevitable. So
I suppose now's a good a time as any to rip this Band Aid clean off. And
having myself on occasion gamed out similar changes to these, I can
somewhat appreciate what appears to be an earnest attempt at leaving a
relatively light footprint overall.

> * lisp/erc/erc-backend.el (erc-networks--id, erc-reuse-buffers,
> erc-kill-server-buffer-on-quit, erc-insert-marker, erc-input-marker,
> erc-hide-prompt, erc-default-recipients, erc-prompt-hidden,

Is the convention here really to list all of these? Or is this like all
part of the show (or maybe both)? (Sorry, I, Philistine.) If yes to the
first, I guess I've been missing out but will fall in line ASAP. FWIW,
grepping the change logs for "forward decl" only returns a handful of
hits.

> @@ -1673,12 +1880,15 @@ erc--parse-isupport-value
>           (split-string value ",")
>         (list value)))))
>
> -;; FIXME move to erc-compat (once we decide how to load it)
> -(defalias 'erc--with-memoization
> -  (cond
> -   ((fboundp 'with-memoization) #'with-memoization) ; 29.1
> -   ((fboundp 'cl--generic-with-memoization) #'cl--generic-with-memoization)
> -   (t (lambda (_ v) v))))

Hmm, yeah, thanks. Caught that one on the chin on the way out the door
and was hoping no one would notice until I could sweep it under the rug
all furtive like. (It's definitely one for the ages, though, I'll give
you that: hall of shame material for sure.) I'm kinda surprised the
compiled version runs at all instead of signaling an "invalid function"
or something. Either way, short of our keeping it as a pet and renaming
it `with-bath-salts' (or `with-kung-flu' or whatever), I doubt it'll be
long for this world in any form once we get our compat ducks in a row.

> +(declare-function erc-nickname-in-use "erc")
>  (define-erc-response-handler (433)
> -  "Login-time \"nick in use\"." nil
> -  (erc-nickname-in-use (cadr (erc-response.command-args parsed))
> -                       "already in use"))
> +                             "Login-time \"nick in use\"." nil
> +                             (erc-nickname-in-use (cadr 
> (erc-response.command-args parsed))
> +                                                  "already in use"))

Indentation thing aside, I wasn't aware we were allowed to forgo
including signatures in these `declare-function' forms. But if that's
the deal, I guess it makes for a cleaner, less cluttered look overall.

>From "Rework mutual dependency yada yada" (bug#54825) [2]:

  >  ;;;; Variables and options
  > modified   lisp/erc/erc.el
  > @@ -130,7 +130,26 @@ erc-scripts
  >    "Running scripts at startup and with /LOAD."
  >    :group 'erc)
  >
  > -(require 'erc-backend)
  > +;; Defined in erc-backend
  > +(defvar erc--server-reconnecting)
  > +(defvar erc-channel-members-changed-hook)
  > +(defvar erc-server-367-functions)
  > +(defvar erc-server-announced-name)
  > [...]
  >
  > @@ -48,6 +48,27 @@ erc--read-time-period
  >    (cl-letf (((symbol-function 'read-string) (lambda (&rest _) "1d")))
  >      (should (equal (erc--read-time-period "foo: ") 86400))))
  >
  > +(ert-deftest erc--meta--backend-dependencies ()
  > +  (with-temp-buffer
  > +    (insert-file-contents-literally

Your changes obsolete most of these `defvars' and the associated test.
Not saying it should fall on you to tidy up, but "someone" should remove
them. Which gets me wondering (not for the first time): all these
`defvar's and `declare-function's seem a bit litter-prone, no? Not sure
if any diagnostic tooling already catches extraneous ones orphaned by
refactoring, but I for one could use such a feature.

> Don't do this.
>
> Asking people to order require's is about as effective
> as asking kids to keep off the grass.

How about a more pedestrian commit subject, like maybe one mentioning
ERC or something it defines? (But just rolling the dice on whatever
malaprop-laden ESL lesson I cook up also works.)

> @@ -196,7 +209,8 @@ erc-send-distinguish-noncommands
>                 (not (string-match "\n.+$" string))
>                 (memq cmd-fun erc-noncommands-list))
>        ;; Inhibit sending this string.
> -      (setf (erc-input-insertp state) nil))))
> +      (with-no-warnings ;how to declare-function a cl-defmethod?
> +        (setf (erc-input-insertp state) nil)))))
>
>  ;;; IRC control character processing.
>  (defgroup erc-control-characters nil

When I byte compile erc-goodies.el and then, with emacs -Q, do

  (require 'erc)

  (let ((input (make-erc-input :string "/me nods" :sendp t :insertp t)))
    (erc-send-distinguish-noncommands input)
    input)

I get

  Symbol’s function definition is void: \(setf\ erc-input-insertp\)

Adding this somewhere atop goodies

  (eval-when-compile
    (provide 'erc-goodies)
    (require 'erc))

seems to give the expected output:

  => #s(erc-input "/me nods" nil t)

Although it also introduces a couple new compiler warnings as well, so
what do I know? (Don't answer that.) This or similar (in emacs -Q) may
offer some insights:

  (trace-function-foreground 'gv-get)
  (byte-compile-file "lisp/erc/erc-goodies.el")

Basically, I don't really care what happens with `erc-goodies' in the
extreme short term (barring some immediate ELPA release) as long as we
preserve as much behavior and blame as is reasonable. Hopefully,
requiring goodies will become a thing of the past after this [3] or
something superior lands, meaning expect new bugs from me shortly.
(That's your cue to flee, people.)

Thanks,
J.P.

    
[1] Not that anyone should care, but there was, for example, the matter
    of damage control for some unfortunate advice that found its way to
    a couple "exemplar" configs, which ended up hooking all their setup
    on `erc-backend' as a sad workaround for these ancient dependency
    problems. So, some of my aim might have been diverted toward not
    breaking those until we had a worthy feature to trade for the churn.
    Add to that a general reticence to disturb large swaths of ancient
    blame and we get closer to my (perhaps failed) attempt at reining in
    only the most unwieldy parts of the dependency cycle rather than
    doing the bold thing (as is being proposed here).

[2] https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=0f52e7ac

[3] https://ttm.sh/es3