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

Previous Next

Full log

Message #8 received at 43413 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Alex Bochannek <alex <at> bochannek.com>
Cc: 43413 <at> debbugs.gnu.org
Subject: Re: bug#43413: 28.0.50; [PATCH] New gnus-score-func to support
 user-defined scoring functions
Date: Tue, 15 Sep 2020 14:50:15 +0200

Alex Bochannek <alex <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.