bug#43413: 28.0.50; [PATCH] New gnus-score-func to support user-defined scoring functions

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> Although it's only ~40 lines of Elisp and ~30 lines of Texinfo, I am
> pretty sure it's the largest code change I have submitted to Emacs and I
> would not be surprised if I violated some coding standards. I have spent
> a fair amount of time with testing, but cannot rule out corner cases, of
> course. Let me know if you want me to make any improvements before
> accepting this patch.

Looks pretty good, but the main problem is neglecting to let-bind
variables.  byte-compiling is a good way to catch these errors:

In gnus-score-edit-file-at-point:
gnus/gnus-score.el:1190:23: Warning: assignment to free variable `move'
gnus/gnus-score.el:1190:23: Warning: reference to free variable `move'

In gnus-score-func:
gnus/gnus-score.el:1657:35: Warning: assignment to free variable `articles'
gnus/gnus-score.el:1654:36: Warning: assignment to free variable `alist'
gnus/gnus-score.el:1669:46: Warning: reference to free variable `alist'
gnus/gnus-score.el:1655:28: Warning: assignment to free variable `entries'
gnus/gnus-score.el:1655:28: Warning: reference to free variable `entries'
gnus/gnus-score.el:1670:35: Warning: reference to free variable `articles'
gnus/gnus-score.el:1667:32: Warning: assignment to free variable `art'
gnus/gnus-score.el:1659:22: Warning: reference to free variable `art'
gnus/gnus-score.el:1665:53: Warning: assignment to free variable
    `article-alist'
gnus/gnus-score.el:1665:67: Warning: assignment to free variable `score'
gnus/gnus-score.el:1665:67: Warning: reference to free variable
    `article-alist'
gnus/gnus-score.el:1666:34: Warning: reference to free variable `score'
gnus/gnus-score.el:1666:40: Warning: assignment to free variable `fn-score'
gnus/gnus-score.el:1670:44: Warning: reference to free variable `fn-score'

In end of data:
gnus/gnus-score.el:3146:1: Warning: the function `cl-pairlis' might not be
    defined at runtime.

> +     (if (string-match "(.*)" rule)
> +         (setq move 0) (setq move -1))

Even if the branches here are short, we prefer to write that as

        (if (string-match "(.*)" rule)
            (setq move 0)
          (setq move -1))

Or even better:

(setq move
      (if (string-match "(.*)" rule)
          0
        -1))

              
> +    (dolist (score-fn (cdr entries))
> +      (let ((score-fn (car score-fn)))
> +         (while (setq art (pop articles))

And this could probably be a

  (dolist (art articles)


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no