bug#59458: [PATCH] Fix tracing for advanced scoring

bug#59458: [PATCH] Fix tracing for advanced scoring

[Łukasz Stelmach]

* lisp/gnus/gnus-logic.el (gnus-score-advanced): Move the tracing
code outside of if so it's executed for both branches.
---
 lisp/gnus/gnus-logic.el | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lisp/gnus/gnus-logic.el b/lisp/gnus/gnus-logic.el
index c1b559ba6f4..346d8a28910 100644
--- a/lisp/gnus/gnus-logic.el
+++ b/lisp/gnus/gnus-logic.el
@@ -71,11 +71,11 @@
 		    (+ (cdr score) new-score))
 	  (push (cons (mail-header-number gnus-advanced-headers)
 		      new-score)
-		gnus-newsgroup-scored)
-	  (when trace
-	    (push (cons "A file" rule)
-		  ;; Must be synced with `gnus-score-edit-file-at-point'.
-		  gnus-score-trace)))))))
+		gnus-newsgroup-scored))
+	(when trace
+	  (push (cons "A file" rule)
+		;; Must be synced with `gnus-score-edit-file-at-point'.
+		gnus-score-trace))))))
 
 (defun gnus-advanced-score-rule (rule)
   "Apply RULE to `gnus-advanced-headers'."
-- 
2.30.2

bug#59458: [PATCH] Fix tracing for advanced scoring

[Eli Zaretskii]

> Cc: Łukasz Stelmach <stlman@poczta.fm>
> From: Łukasz Stelmach <stlman@poczta.fm>
> Date: Mon, 21 Nov 2022 22:30:55 +0100
> 
> * lisp/gnus/gnus-logic.el (gnus-score-advanced): Move the tracing
> code outside of if so it's executed for both branches.
> ---
>  lisp/gnus/gnus-logic.el | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/lisp/gnus/gnus-logic.el b/lisp/gnus/gnus-logic.el
> index c1b559ba6f4..346d8a28910 100644
> --- a/lisp/gnus/gnus-logic.el
> +++ b/lisp/gnus/gnus-logic.el
> @@ -71,11 +71,11 @@
>  		    (+ (cdr score) new-score))
>  	  (push (cons (mail-header-number gnus-advanced-headers)
>  		      new-score)
> -		gnus-newsgroup-scored)
> -	  (when trace
> -	    (push (cons "A file" rule)
> -		  ;; Must be synced with `gnus-score-edit-file-at-point'.
> -		  gnus-score-trace)))))))
> +		gnus-newsgroup-scored))
> +	(when trace
> +	  (push (cons "A file" rule)
> +		;; Must be synced with `gnus-score-edit-file-at-point'.
> +		gnus-score-trace))))))
>  
>  (defun gnus-advanced-score-rule (rule)
>    "Apply RULE to `gnus-advanced-headers'."
> -- 
> 2.30.2

Lars, Eric,

Any comments?  Is this good to go in?

Thanks.

bug#59458: [PATCH] Fix tracing for advanced scoring

[Eric Abrahamsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Łukasz Stelmach <stlman@poczta.fm>
>> From: Łukasz Stelmach <stlman@poczta.fm>
>> Date: Mon, 21 Nov 2022 22:30:55 +0100
>> 
>> * lisp/gnus/gnus-logic.el (gnus-score-advanced): Move the tracing
>> code outside of if so it's executed for both branches.

I'm not very familiar with this code (this is actually the first I'm
hearing of gnus-logic.el), so I hope Lars will chime in. It makes sense
that tracing should happen whether or not the rule matched? But what
about the sexp before that? Would we be pushing the mail-header-number
and new score to `gnus-newsgroup-score' only if the rule *wasn't*
successful?

I think this one should wait for Lars. If we don't hear from him and
it's holding things up, I can look more closely.

bug#59458: [PATCH] Fix tracing for advanced scoring

[Stefan Kangas]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: Łukasz Stelmach <stlman@poczta.fm>
>>> From: Łukasz Stelmach <stlman@poczta.fm>
>>> Date: Mon, 21 Nov 2022 22:30:55 +0100
>>>
>>> * lisp/gnus/gnus-logic.el (gnus-score-advanced): Move the tracing
>>> code outside of if so it's executed for both branches.
>
> I'm not very familiar with this code (this is actually the first I'm
> hearing of gnus-logic.el), so I hope Lars will chime in. It makes sense
> that tracing should happen whether or not the rule matched? But what
> about the sexp before that? Would we be pushing the mail-header-number
> and new score to `gnus-newsgroup-score' only if the rule *wasn't*
> successful?
>
> I think this one should wait for Lars. If we don't hear from him and
> it's holding things up, I can look more closely.

Eric,

It would be great if you could help review this.  Thanks in advance.

bug#59458: [PATCH] Fix tracing for advanced scoring

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Thu, 7 Sep 2023 14:07:57 -0700
> Cc: Eli Zaretskii <eliz@gnu.org>, Lars Ingebrigtsen <larsi@gnus.org>, 59458@debbugs.gnu.org, 
> 	Łukasz Stelmach <stlman@poczta.fm>
> 
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
> 
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> >>> Cc: Łukasz Stelmach <stlman@poczta.fm>
> >>> From: Łukasz Stelmach <stlman@poczta.fm>
> >>> Date: Mon, 21 Nov 2022 22:30:55 +0100
> >>>
> >>> * lisp/gnus/gnus-logic.el (gnus-score-advanced): Move the tracing
> >>> code outside of if so it's executed for both branches.
> >
> > I'm not very familiar with this code (this is actually the first I'm
> > hearing of gnus-logic.el), so I hope Lars will chime in. It makes sense
> > that tracing should happen whether or not the rule matched? But what
> > about the sexp before that? Would we be pushing the mail-header-number
> > and new score to `gnus-newsgroup-score' only if the rule *wasn't*
> > successful?
> >
> > I think this one should wait for Lars. If we don't hear from him and
> > it's holding things up, I can look more closely.
> 
> Eric,
> 
> It would be great if you could help review this.  Thanks in advance.

Adding Andrew, in case he could have some ideas or comments.

bug#59458: [PATCH] Fix tracing for advanced scoring

[Eric Abrahamsen]

On 09/07/23 14:07 PM, Stefan Kangas wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Cc: Łukasz Stelmach <stlman@poczta.fm>
>>>> From: Łukasz Stelmach <stlman@poczta.fm>
>>>> Date: Mon, 21 Nov 2022 22:30:55 +0100
>>>>
>>>> * lisp/gnus/gnus-logic.el (gnus-score-advanced): Move the tracing
>>>> code outside of if so it's executed for both branches.
>>
>> I'm not very familiar with this code (this is actually the first I'm
>> hearing of gnus-logic.el), so I hope Lars will chime in. It makes sense
>> that tracing should happen whether or not the rule matched? But what
>> about the sexp before that? Would we be pushing the mail-header-number
>> and new score to `gnus-newsgroup-score' only if the rule *wasn't*
>> successful?
>>
>> I think this one should wait for Lars. If we don't hear from him and
>> it's holding things up, I can look more closely.
>
> Eric,
>
> It would be great if you could help review this.  Thanks in advance.

I've taken a closer look, and I do think this is okay. The `when' after
the `dolist' only fires if the rule matches, the `if' is only there to
see if this article has been previously scored or not. So moving the
"(when trace" up to the top-level of the containing `when' (ie, out of the
`if') does look like the right thing to do.

Eric

bug#59458: [PATCH] Fix tracing for advanced scoring

[Stefan Kangas]

Version: 30.1

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> On 09/07/23 14:07 PM, Stefan Kangas wrote:
>
>> Eric,
>>
>> It would be great if you could help review this.  Thanks in advance.
>
> I've taken a closer look, and I do think this is okay. The `when' after
> the `dolist' only fires if the rule matches, the `if' is only there to
> see if this article has been previously scored or not. So moving the
> "(when trace" up to the top-level of the containing `when' (ie, out of the
> `if') does look like the right thing to do.

Thanks.  Installed it on master as commit e25ad6e2a30.