On Wed, Jun 08 2022, David Bremner wrote:

> jao <jao@gnu.org> writes:
>
>>
>>  
>> +(defun notmuch-mua--remove-dont-reply-to-names ()
>> +  (when-let ((nr (message-dont-reply-to-names)))
>
> Using when-let is fine with me, but I wonder if we should follow the
> advice in subr-x to do
>
>        (eval-when-compile (require 'subr-x))
>
> It's autoloaded, but maybe we can improve loading time a little?
> I noticed the other places we just do (require 'subr-x), but we're not
> always the most *cough* sophisticated elisp programmers.

well, i'd be surprised if it made any noticeable difference, but it
surely won't hurt.  added.

> I'm a bit nervous about calling an undocumented defsubst here. It might
> be better to inline the 3 line definition. What do you think?

in all fairness, its name doesn't include a double dash, so that makes
it theoretically public.  but it's true it's undocumented, and, if
inlining makes the code clearer without having to add explanatory
messages, i think inlining is a net win. done.

>> +      (dolist (header '("To" "Cc"))
>
> I found the re-use of v confusing in this 'when-let'. It reminds me of
> an exam question about scope :).

i'll change it, no problem.

>
>> +	(when-let ((v (message-fetch-field header)))
>> +	  (let* ((v (mapcar #'string-trim (message-tokenize-header v)))
>> +		 (vs (delq nil (mapcar nr-filter v)))
>> +		 (v (when vs (mapconcat #'identity vs ", "))))
>
> I think 'and' is conventional for places where we care about what the
> expression evaluates to, and 'when' preferred when not.

fair enough.  i've removed also similar usages of 'unless'.

[...]

>> @@ -0,0 +1,76 @@
>> +#!/usr/bin/env bash
>> +
>> +test_description="emacs reply"
>
> I guess this description should match the name of the file and mention
> something like "don't reply"; this will make the individual test
> descriptions make more sense

oops yes, forgot to update it.

>> +. $(dirname "$0")/test-lib.sh || exit 1
>> +. $NOTMUCH_SRCDIR/test/test-lib-emacs.sh || exit 1
>> +
>> +EXPECTED=$NOTMUCH_SRCDIR/test/emacs-show.expected-output
>> +
>> +test_require_emacs
>> +
>> +add_email_corpus default
>> +
>> +test_begin_subtest "regular expression"
>> +test_emacs '(let ((message-dont-reply-to-names "notmuchmail\\|noreply\\|harvard"))
>> +	      (notmuch-mua-new-reply
>> +	        "id:20091117203301.GV3165@dottiness.seas.harvard.edu" nil t)
>> +	      (test-visible-output "OUTPUT-FULL.raw"))'
>> +
>
> Maybe mention this tests Cc; I don't _think_ the other tests do?

they actually do: they make it disappear!

i'm submitting a new version with the changes above: let me know how it
looks.

jao
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org