GNU bug report logs - #57548
Add new function `seq-positions'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 57548 in the body.
You can then email your comments to 57548 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Damien Cassou <damien <at> cassou.me>
To: bug-gnu-emacs <at> gnu.org
Subject: Add new function `seq-positions'
Date: Fri, 02 Sep 2022 20:43:24 +0200

[Message part 1 (text/plain, inline)]

Tags: patch

Hi,

here is a patch adding seq-positions to seq.el.

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

[0001-Add-new-function-seq-positions.patch (text/patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Damien Cassou <damien <at> cassou.me>
Cc: 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Fri, 02 Sep 2022 22:00:14 +0300

> From: Damien Cassou <damien <at> cassou.me>
> Date: Fri, 02 Sep 2022 20:43:24 +0200
> 
> +@defun seq-positions sequence elt &optional testfn
> +  This function returns a list of the positions of the elements in
> +@var{sequence} that are equal to @var{elt}.

We use "index", not "position".  In any case, the documentation should
explain what you mean by that, and it should definitely say that the
index/position are zero-based.

> +@example
> +@group
> +(seq-positions '(a b c a d) 'a)
> +@result{} (0 3)
> +@end group
> +@group
> +(seq-position '(a b c a d) 'z)
> +@result{} nil

seq-position or seq-positions?

> +(cl-defgeneric seq-positions (sequence elt &optional testfn)
> +  "Return a list of the positions of ELT in SEQ.
> +Equality is defined by TESTFN if non-nil or by `equal' if nil."

Our style is to say

  Equality is defined by the function TESTFN, which defaults to `equal'.

> --- a/test/lisp/emacs-lisp/seq-tests.el
> +++ b/test/lisp/emacs-lisp/seq-tests.el
> @@ -482,6 +482,11 @@ test-seq-position
>      (should (= (seq-position seq 'a #'eq) 0))
>      (should (null (seq-position seq (make-symbol "a") #'eq)))))
>  
> +(ert-deftest test-seq-positions ()
> +  (with-test-sequences (seq '(1 2 3 1 4))
> +    (should (equal '(0 3) (seq-positions seq 1)))
> +    (should (seq-empty-p (seq-positions seq 9)))))

Should we test more than just one type of sequences?

Thanks.

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Damien Cassou <damien <at> cassou.me>
Cc: 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Sat, 03 Sep 2022 03:42:02 +0200

Damien Cassou <damien <at> cassou.me> writes:

> here is a patch adding seq-positions to seq.el.

+@defun seq-positions sequence elt &optional testfn
+  This function returns a list of the positions of the elements in
+@var{sequence} that are equal to @var{elt}.  If the optional argument
+@var{testfn} is non-@code{nil}, it is a function of two arguments to
+use instead of the default @code{equal}.

We do not need to limit this to equivalence relations.  A TESTFUN of, say,
(apply-partially #'<= 10) could be similarly useful.

+;;;###autoload
+(cl-defgeneric seq-positions (sequence elt &optional testfn)
+  "Return a list of the positions of ELT in SEQ.
+Equality is defined by TESTFN if non-nil or by `equal' if nil."
+  (let ((result '())
+        (index 0))
+    (seq-doseq (e sequence)
+      (when (funcall (or testfn #'equal) e elt)
+        (push index result))
+      (setq index (1+ index)))
+    (nreverse result)))

Could this maybe (simpler) call `seq-do-indexed'?


TIA,

Michael.

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

From: Damien Cassou <damien <at> cassou.me>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Sat, 03 Sep 2022 10:01:26 +0200

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:
> We use "index", not "position".

I changed the text in the manual, NEWS and docstring to talk about
"index" instead of "position".  I kept the word "positions" in the
function name because there is already a `seq-position` function and the
2 are so similar that I think they deserve a similar name. What do you
think?


> In any case, the documentation should explain what you mean by that,


I haven't found another such explanation in seq.texi so I'm not sure
what you means.  I would be happy to write something if you feel
something is still missing though.


> and it should definitely say that the index/position are zero-based.


done in the manual, NEWS and docstring.


>> +@group
>> +(seq-position '(a b c a d) 'z)
>> +@result{} nil
>
> seq-position or seq-positions?


you are so right.  Thank you for the careful review.


> Our style is to say
>   Equality is defined by the function TESTFN, which defaults to `equal'.


fixed.  If you want, I can prepare another patch to apply the same
change to the docstring of the already existing `seq-position`: it
contains the same phrasing.


>> +(ert-deftest test-seq-positions ()
>> +  (with-test-sequences (seq '(1 2 3 1 4))
>> +    (should (equal '(0 3) (seq-positions seq 1)))
>> +    (should (seq-empty-p (seq-positions seq 9)))))
>
> Should we test more than just one type of sequences?


The `with-test-sequences` call checks 3 types of sequences already as
far as I understand. Do you mean something else?


Michael Heerdegen <michael_heerdegen <at> web.de> writes:
> We do not need to limit this to equivalence relations.  A TESTFUN of, say,
> (apply-partially #'<= 10) could be similarly useful.


Indeed, such a function would certainly make sense.  I would refrain
from calling it `seq-positions` though because there is already a
`seq-position` function that is very similar.  I guess the function you
suggest wouldn't take an element as argument either.  In any case, I
think the current function makes sense on its own and what you are
asking for is a new function that seems out of scope of this current
patch.  If you give me a name (what about `(seq-matching-positions seq
pred)`?), I volunteer to send a new patch in a new mail thread.

Best

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

[0001-Add-new-function-seq-positions.patch (text/x-patch, attachment)]

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

From: Damien Cassou <damien <at> cassou.me>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Sat, 03 Sep 2022 10:02:33 +0200

Michael Heerdegen <michael_heerdegen <at> web.de> writes:
> Could this maybe (simpler) call `seq-do-indexed'?

excellent suggestion, thank you. I applied it in the second version of
the patch. I'm sorry I didn't mention it there.

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Damien Cassou <damien <at> cassou.me>
Cc: 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Sat, 03 Sep 2022 13:16:32 +0300

> From: Damien Cassou <damien <at> cassou.me>
> Cc: 57548 <at> debbugs.gnu.org
> Date: Sat, 03 Sep 2022 10:01:26 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> > We use "index", not "position".
> 
> I changed the text in the manual, NEWS and docstring to talk about
> "index" instead of "position".  I kept the word "positions" in the
> function name because there is already a `seq-position` function and the
> 2 are so similar that I think they deserve a similar name. What do you
> think?

LGTM, thanks.

> > In any case, the documentation should explain what you mean by that,
> 
> I haven't found another such explanation in seq.texi so I'm not sure
> what you means.  I would be happy to write something if you feel
> something is still missing though.

As you changed the text to talk about indices, I don't think anything
else is needed.

> > Our style is to say
> >   Equality is defined by the function TESTFN, which defaults to `equal'.
> 
> fixed.  If you want, I can prepare another patch to apply the same
> change to the docstring of the already existing `seq-position`: it
> contains the same phrasing.

Yes, please.  Will be appreciated.

> >> +(ert-deftest test-seq-positions ()
> >> +  (with-test-sequences (seq '(1 2 3 1 4))
> >> +    (should (equal '(0 3) (seq-positions seq 1)))
> >> +    (should (seq-empty-p (seq-positions seq 9)))))
> >
> > Should we test more than just one type of sequences?
> 
> 
> The `with-test-sequences` call checks 3 types of sequences already as
> far as I understand. Do you mean something else?

No, I've missed that part

Thanks.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Damien Cassou <damien <at> cassou.me>, 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Sat, 03 Sep 2022 14:27:42 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

>> I changed the text in the manual, NEWS and docstring to talk about
>> "index" instead of "position".  I kept the word "positions" in the
>> function name because there is already a `seq-position` function and the
>> 2 are so similar that I think they deserve a similar name. What do you
>> think?
>
> LGTM, thanks.

Me too.

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

From: Damien Cassou <damien <at> cassou.me>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Sat, 03 Sep 2022 15:03:02 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:
> From: Damien Cassou <damien <at> cassou.me>
>> If you want, I can prepare another patch to apply the same change to
>> the docstring of the already existing `seq-position`: it contains the
>> same phrasing.
>
> Yes, please.  Will be appreciated.

fixed in bug#57561.

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Damien Cassou <damien <at> cassou.me>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Sun, 04 Sep 2022 04:27:17 +0200

Damien Cassou <damien <at> cassou.me> writes:

> Indeed, such a function would certainly make sense.  I would refrain
> from calling it `seq-positions` though because there is already a
> `seq-position` function that is very similar.  I guess the function you
> suggest wouldn't take an element as argument either.  In any case, I
> think the current function makes sense on its own and what you are
> asking for is a new function that seems out of scope of this current
> patch.  If you give me a name (what about `(seq-matching-positions seq
> pred)`?), I volunteer to send a new patch in a new mail thread.

No - I actually mean (and, sorry, I see that my initial response said
something slightly different): calling the function with ELT == 10 and
TESTFN == #'>= would return all positions in the SEQUENCE of elements
not less than 10.

We don't need another function.  You only need to say that TESTFN is not
necessarily an equality predicate or symmetric.  And in which order the
two arguments are passed (and have a look that your current calling order
is so that the above example works as expected).

Michael.

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

From: Damien Cassou <damien <at> cassou.me>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Sun, 04 Sep 2022 10:44:12 +0200

[Message part 1 (text/plain, inline)]

Michael Heerdegen <michael_heerdegen <at> web.de> writes:
> We don't need another function.  You only need to say that TESTFN is not
> necessarily an equality predicate or symmetric.  And in which order the
> two arguments are passed (and have a look that your current calling order
> is so that the above example works as expected).

Very clear. See attached patch for the new version. I also added a unit
test to make sure the result is what you expect.

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

[0001-Add-new-function-seq-positions.patch (text/x-patch, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Damien Cassou <damien <at> cassou.me>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, Eli Zaretskii <eliz <at> gnu.org>,
 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Sun, 04 Sep 2022 13:22:13 +0200

Damien Cassou <damien <at> cassou.me> writes:

> Very clear. See attached patch for the new version. I also added a unit
> test to make sure the result is what you expect.

Thanks; pushed to Emacs 29.

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Damien Cassou <damien <at> cassou.me>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 57548 <at> debbugs.gnu.org
Subject: Re: bug#57548: Add new function `seq-positions'
Date: Tue, 06 Sep 2022 03:44:13 +0200

Damien Cassou <damien <at> cassou.me> writes:

> Very clear. See attached patch for the new version. I also added a unit
> test to make sure the result is what you expect.

Ok, great, thanks.

> +TESTFN is a two-argument function which is passed each element of
> +SEQUENCE as first argument and ELT as second. TESTFN defaults to
> +`equal'.

I hope this order feels natural for everyone?  `pred` in `pcase' does
the opposite, but there the predicate comes first (syntactically).
We don't need to have the order analogous to pcase - but if people
prefer ELT being the first element in the TESTFN call, we can still
change it.

Michael.

Previous Next

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