GNU bug report logs - #34852
26.1; seq-intersection ignores nil as element

Previous Next

Full log

View this message in rfc822 format

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Nicolas Petton <nicolas <at> petton.fr>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, 34852 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>, "Miguel V. S. Frasson" <mvsfrasson <at> gmail.com>
Subject: bug#34852: 26.1; seq-intersection ignores nil as element
Date: Thu, 21 Mar 2019 17:46:51 +0000

Nicolas Petton <nicolas <at> petton.fr> writes:

> Here's a patch for master.

Thanks, I have only some minor comments:

> diff --git a/lisp/emacs-lisp/seq.el b/lisp/emacs-lisp/seq.el
> index 4a811d7895..39c93e25ed 100644
> --- a/lisp/emacs-lisp/seq.el
> +++ b/lisp/emacs-lisp/seq.el
> @@ -356,6 +356,7 @@ seq-sort-by
>      count))
>  
>  (cl-defgeneric seq-contains (sequence elt &optional testfn)
> +  (declare (obsolete "Use `seq-contains-p' instead." "27.1"))

According to make-obsolete, "use instead" strings should not start with
a capital letter, but even better is (obsolete seq-contains-p "27.1"),
which adds a link to seq-contains-p in the *Help* buffer.

> @@ -420,7 +430,7 @@ seq-sort-by
>    "Return a list of the elements that appear in SEQUENCE1 but not in SEQUENCE2.
>  Equality is defined by TESTFN if non-nil or by `equal' if nil."
>    (seq-reduce (lambda (acc elt)
> -                (if (not (seq-contains sequence2 elt testfn))
> +                (if (not (seq-contains-p sequence2 elt testfn))
>                      (cons elt acc)
>                    acc))

Is there any harm in inverting this conditional structure, so that it
reads positively?

> diff --git a/test/lisp/emacs-lisp/seq-tests.el b/test/lisp/emacs-lisp/seq-tests.el
> index d8f00cfea4..6522def423 100644
> --- a/test/lisp/emacs-lisp/seq-tests.el
> +++ b/test/lisp/emacs-lisp/seq-tests.el
> @@ -185,6 +185,14 @@ test-sequences-oddp
>    (with-test-sequences (seq '(3 4 5 6))
>      (should (= 5 (seq-contains seq 5)))))
>  
> +(ert-deftest test-seq-contains-p ()
> +  (with-test-sequences (seq '(3 4 5 6))
> +    (should (eq (seq-contains-p seq 3) t))
> +    (should-not (seq-contains-p seq 7)))
> +  (with-test-sequences (seq '())
> +    (should-not (seq-contains-p seq 3))
> +    (should-not (seq-contains-p seq nil))))
> +

I think there should also be an explicit nil membership check:

(should (seq-contains-p  [nil] nil))
(should (seq-contains-p '(nil) nil))

-- 
Basil

Previous Next

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