unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
@ 2022-11-17  2:17 Michael Heerdegen
  2022-11-19 13:12 ` Eli Zaretskii
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Heerdegen @ 2022-11-17  2:17 UTC (permalink / raw)
  To: 59328; +Cc: Lars Ingebrigtsen, Jonas Bernoulli


Hello,

  [FWIW I tried to reopen 58278 but it seemed to...complicated for me]

The current implementation of the (non-generic!) function `seq-keep':

#+begin_src emacs-lisp
(defun seq-keep (function sequence)
  (delq nil (seq-map function sequence)))
;; ^^^^
#+end_src

obviously only works when `seq-map' returns a list.  This is the case
for the default implementation of the generic function `seq-map' but not
necessarily for other implementations of `seq-map'.

We need to filter out the `nil' elements with a way appropriate for any
sequence type supported by "seq.el" (i.e. with a generic function
defined in this lib), e.g.

#+begin_src emacs-lisp
(defun seq-keep (function sequence)
  (seq-filter #'identity (seq-map function sequence)))
#+end_src


BTW, is the name a good one?  Why "keep"?  It returns a sequence of
potentially all completely different elements.  And is the function that
useful and a good abstraction at all (I don't have thought about it
too long...)?


TIA,

Michael.







^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-17  2:17 bug#59328: 29.0.50; `seq-keep' implementation only valid for lists Michael Heerdegen
@ 2022-11-19 13:12 ` Eli Zaretskii
  2022-11-24 13:04   ` Michael Heerdegen
  0 siblings, 1 reply; 13+ messages in thread
From: Eli Zaretskii @ 2022-11-19 13:12 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: larsi, jonas, 59328

> Cc: Lars Ingebrigtsen <larsi@gnus.org>, Jonas Bernoulli <jonas@bernoul.li>
> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Thu, 17 Nov 2022 03:17:46 +0100
> 
> The current implementation of the (non-generic!) function `seq-keep':
> 
> #+begin_src emacs-lisp
> (defun seq-keep (function sequence)
>   (delq nil (seq-map function sequence)))
> ;; ^^^^
> #+end_src
> 
> obviously only works when `seq-map' returns a list.  This is the case
> for the default implementation of the generic function `seq-map' but not
> necessarily for other implementations of `seq-map'.
> 
> We need to filter out the `nil' elements with a way appropriate for any
> sequence type supported by "seq.el" (i.e. with a generic function
> defined in this lib), e.g.
> 
> #+begin_src emacs-lisp
> (defun seq-keep (function sequence)
>   (seq-filter #'identity (seq-map function sequence)))
> #+end_src

This makes sense to me, so please go ahead and install, preferably
with a test for non-list cases.

> BTW, is the name a good one?  Why "keep"?  It returns a sequence of
> potentially all completely different elements.  And is the function that
> useful and a good abstraction at all (I don't have thought about it
> too long...)?

FWIW, "keep" doesn't sound problematic to me.

Thanks.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-19 13:12 ` Eli Zaretskii
@ 2022-11-24 13:04   ` Michael Heerdegen
  2022-11-24 14:19     ` Eli Zaretskii
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Heerdegen @ 2022-11-24 13:04 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, jonas, 59328

Eli Zaretskii <eliz@gnu.org> writes:

> > #+begin_src emacs-lisp
> > (defun seq-keep (function sequence)
> >   (seq-filter #'identity (seq-map function sequence)))
> > #+end_src
>
> This makes sense to me, so please go ahead and install, preferably
> with a test for non-list cases.

I don't think adding a test case to Emacs is possible - all currently
existing counterexamples I found (stream.el, myers.el) are in Elpa or
somewhere else, but not in the Emacs repo.  So I guess it's ok to go
without a test?

Michael.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-24 13:04   ` Michael Heerdegen
@ 2022-11-24 14:19     ` Eli Zaretskii
  2022-11-24 14:26       ` Michael Heerdegen
  0 siblings, 1 reply; 13+ messages in thread
From: Eli Zaretskii @ 2022-11-24 14:19 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: larsi, jonas, 59328

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 59328@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li
> Date: Thu, 24 Nov 2022 14:04:27 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > #+begin_src emacs-lisp
> > > (defun seq-keep (function sequence)
> > >   (seq-filter #'identity (seq-map function sequence)))
> > > #+end_src
> >
> > This makes sense to me, so please go ahead and install, preferably
> > with a test for non-list cases.
> 
> I don't think adding a test case to Emacs is possible - all currently
> existing counterexamples I found (stream.el, myers.el) are in Elpa or
> somewhere else, but not in the Emacs repo.

I don't understand: why cannot we add a test to Emacs, even though other
tests are elsewhere?

> So I guess it's ok to go without a test?

It's okay, but I'd prefer to have a test.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-24 14:19     ` Eli Zaretskii
@ 2022-11-24 14:26       ` Michael Heerdegen
  2022-11-24 15:00         ` Eli Zaretskii
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Heerdegen @ 2022-11-24 14:26 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, jonas, 59328

Eli Zaretskii <eliz@gnu.org> writes:

> I don't understand: why cannot we add a test to Emacs, even though
> other tests are elsewhere?

Every kind of sequence (in the sense of seq.el) that exists in Emacs is
not affected by the bug.  A test makes only sense for types that are,
but those are defined in libraries that are not in Gnu Emacs, only in
Gnu Elpa.  But I can't make the test depend on them.

Michael.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-24 14:26       ` Michael Heerdegen
@ 2022-11-24 15:00         ` Eli Zaretskii
  2022-11-24 15:09           ` Michael Heerdegen
  0 siblings, 1 reply; 13+ messages in thread
From: Eli Zaretskii @ 2022-11-24 15:00 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: larsi, jonas, 59328

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 59328@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li
> Date: Thu, 24 Nov 2022 15:26:01 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't understand: why cannot we add a test to Emacs, even though
> > other tests are elsewhere?
> 
> Every kind of sequence (in the sense of seq.el) that exists in Emacs is
> not affected by the bug.  A test makes only sense for types that are,
> but those are defined in libraries that are not in Gnu Emacs, only in
> Gnu Elpa.  But I can't make the test depend on them.

I see I was confused by the Subject of your bug report.

OK, then just install this, though now I wonder why we need this change...





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-24 15:00         ` Eli Zaretskii
@ 2022-11-24 15:09           ` Michael Heerdegen
  2022-11-24 15:15             ` Eli Zaretskii
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Heerdegen @ 2022-11-24 15:09 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, jonas, 59328

Eli Zaretskii <eliz@gnu.org> writes:

> > Every kind of sequence (in the sense of seq.el) that exists in Emacs is
> > not affected by the bug.  A test makes only sense for types that are,
> > but those are defined in libraries that are not in Gnu Emacs, only in
> > Gnu Elpa.  But I can't make the test depend on them.
>
> I see I was confused by the Subject of your bug report.
>
> OK, then just install this, though now I wonder why we need this change...

Without that change `seq-keep' would error for sequence types like
streams.  Try for example

#+begin_src emacs-lisp
(require 'stream)
(seq-keep
 (lambda (x) (and (<= 0 x) x))
 (stream (list -1 2 -3 4)))
#+end_src

That errors without the proposed change but it's a legitimate call (and
it should return a stream of course).

Michael.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-24 15:09           ` Michael Heerdegen
@ 2022-11-24 15:15             ` Eli Zaretskii
  2022-11-24 15:25               ` Michael Heerdegen
  0 siblings, 1 reply; 13+ messages in thread
From: Eli Zaretskii @ 2022-11-24 15:15 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: larsi, jonas, 59328

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 59328@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li
> Date: Thu, 24 Nov 2022 16:09:32 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > OK, then just install this, though now I wonder why we need this change...
> 
> Without that change `seq-keep' would error for sequence types like
> streams.  Try for example
> 
> #+begin_src emacs-lisp
> (require 'stream)
> (seq-keep
>  (lambda (x) (and (<= 0 x) x))
>  (stream (list -1 2 -3 4)))
> #+end_src

Didn't you just say that 'stream' is not in Emacs?  If I try the above, the
debugger kicks in right on the 'require' line.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-24 15:15             ` Eli Zaretskii
@ 2022-11-24 15:25               ` Michael Heerdegen
  2022-11-24 17:02                 ` Eli Zaretskii
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Heerdegen @ 2022-11-24 15:25 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, jonas, 59328

Eli Zaretskii <eliz@gnu.org> writes:

> > Without that change `seq-keep' would error for sequence types like
> > streams.  Try for example
> >
> > #+begin_src emacs-lisp
> > (require 'stream)
> > (seq-keep
> >  (lambda (x) (and (<= 0 x) x))
> >  (stream (list -1 2 -3 4)))
> > #+end_src
>
> Didn't you just say that 'stream' is not in Emacs?  If I try the above, the
> debugger kicks in right on the 'require' line.

Yes.

But seq-keep is not a generic function, so it would be broken for
sequence types defined elsewhere, and there is no way for those other
sequence types to fix this.  Defining a generic interface for sequences
would not make much sense if it then only supports lists (and maybe
vectors).

So we want to support cases like streams.  Since seq-keep can be
expressed and is semantically equivalent a simple concetanation of
existing sequence operations there is probably no need to define it as
generic function.  We just need to implement it correctly to support any
otherwise supported type of sequences.

Michael.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-24 15:25               ` Michael Heerdegen
@ 2022-11-24 17:02                 ` Eli Zaretskii
  2022-11-25  9:47                   ` Michael Heerdegen
  0 siblings, 1 reply; 13+ messages in thread
From: Eli Zaretskii @ 2022-11-24 17:02 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: larsi, jonas, 59328

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 59328@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li
> Date: Thu, 24 Nov 2022 16:25:44 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > Without that change `seq-keep' would error for sequence types like
> > > streams.  Try for example
> > >
> > > #+begin_src emacs-lisp
> > > (require 'stream)
> > > (seq-keep
> > >  (lambda (x) (and (<= 0 x) x))
> > >  (stream (list -1 2 -3 4)))
> > > #+end_src
> >
> > Didn't you just say that 'stream' is not in Emacs?  If I try the above, the
> > debugger kicks in right on the 'require' line.
> 
> Yes.
> 
> But seq-keep is not a generic function, so it would be broken for
> sequence types defined elsewhere, and there is no way for those other
> sequence types to fix this.  Defining a generic interface for sequences
> would not make much sense if it then only supports lists (and maybe
> vectors).
> 
> So we want to support cases like streams.

Can tests for this be written in a way that they are only run if the
relevant packages are available on the user's system?  If so, I'd prefer to
have that than no tests at all.

Your call.

Thanks.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-24 17:02                 ` Eli Zaretskii
@ 2022-11-25  9:47                   ` Michael Heerdegen
  2022-11-25 11:34                     ` Stefan Kangas
  2022-11-25 11:55                     ` Eli Zaretskii
  0 siblings, 2 replies; 13+ messages in thread
From: Michael Heerdegen @ 2022-11-25  9:47 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, jonas, 59328

Eli Zaretskii <eliz@gnu.org> writes:

> Can tests for this be written in a way that they are only run if the
> relevant packages are available on the user's system?  If so, I'd
> prefer to have that than no tests at all.

I don't know.

Alternatively we could implement `seq-map' for an ad-hoc defined
sequence type and test using that type, e.g. this expression:

#+begin_src emacs-lisp
(progn
  (defvar gensym)
  (let ((gensym (make-symbol "foo")))
    (eval `(cl-defmethod seq-map (function (thing (head ,gensym)))
             (append (list (car thing) (cadr thing)) (seq-map function (cddr thing))))
          t)
    (equal (list gensym nil 4 46)
           (seq-keep (lambda (x) (and (integerp x) (* 2 x)))
                     (list gensym nil 2 'x gensym 23)))))
#+end_src

returns t with my patch installed and nil else and works without relying
on something external.  I'm not sure if defining methods (for seq-map in
this case) that are globally visible is allowed in tests, so I
implemented the example above in a way that the change of the generic
function is not visible from the outside (thus the "secret" gensym).

Would something like that be acceptable?

Sorry for my ignorance, I didn't write much tests before.


TIA,

Michael.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-25  9:47                   ` Michael Heerdegen
@ 2022-11-25 11:34                     ` Stefan Kangas
  2022-11-25 11:55                     ` Eli Zaretskii
  1 sibling, 0 replies; 13+ messages in thread
From: Stefan Kangas @ 2022-11-25 11:34 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: Eli Zaretskii, jonas, 59328, larsi

Michael Heerdegen <michael_heerdegen@web.de> writes:

> #+begin_src emacs-lisp
> (progn
>   (defvar gensym)
>   (let ((gensym (make-symbol "foo")))
>     (eval `(cl-defmethod seq-map (function (thing (head ,gensym)))
>              (append (list (car thing) (cadr thing)) (seq-map function (cddr thing))))
>           t)
>     (equal (list gensym nil 4 46)
>            (seq-keep (lambda (x) (and (integerp x) (* 2 x)))
>                      (list gensym nil 2 'x gensym 23)))))
> #+end_src
>
> returns t with my patch installed and nil else and works without relying
> on something external. I'm not sure if defining methods (for seq-map in
> this case) that are globally visible is allowed in tests, so I
> implemented the example above in a way that the change of the generic
> function is not visible from the outside (thus the "secret" gensym).

We allow things that are globally visible, yes, as that will only
affect testing sessions.  So I think dropping the gensym will be an
improvement.

> Would something like that be acceptable?

Makes sense to me.  Perhaps we should do something similar for other
seq.el tests.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* bug#59328: 29.0.50; `seq-keep' implementation only valid for lists
  2022-11-25  9:47                   ` Michael Heerdegen
  2022-11-25 11:34                     ` Stefan Kangas
@ 2022-11-25 11:55                     ` Eli Zaretskii
  1 sibling, 0 replies; 13+ messages in thread
From: Eli Zaretskii @ 2022-11-25 11:55 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: larsi, jonas, 59328

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 59328@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li
> Date: Fri, 25 Nov 2022 10:47:31 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can tests for this be written in a way that they are only run if the
> > relevant packages are available on the user's system?  If so, I'd
> > prefer to have that than no tests at all.
> 
> I don't know.

AFAIK, 'require' can return nil if asked not to error out.

> Alternatively we could implement `seq-map' for an ad-hoc defined
> sequence type and test using that type, e.g. this expression:
> 
> #+begin_src emacs-lisp
> (progn
>   (defvar gensym)
>   (let ((gensym (make-symbol "foo")))
>     (eval `(cl-defmethod seq-map (function (thing (head ,gensym)))
>              (append (list (car thing) (cadr thing)) (seq-map function (cddr thing))))
>           t)
>     (equal (list gensym nil 4 46)
>            (seq-keep (lambda (x) (and (integerp x) (* 2 x)))
>                      (list gensym nil 2 'x gensym 23)))))
> #+end_src
> 
> returns t with my patch installed and nil else and works without relying
> on something external.  I'm not sure if defining methods (for seq-map in
> this case) that are globally visible is allowed in tests, so I
> implemented the example above in a way that the change of the generic
> function is not visible from the outside (thus the "secret" gensym).
> 
> Would something like that be acceptable?
> 
> Sorry for my ignorance, I didn't write much tests before.

Sounds like over-engineering to me.

Like I said: it's your call.  If you see too many complications to adding a
test, and my suggestions don't convince you, I won't object to installing
your original proposal without a test.

Thanks.





^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-11-25 11:55 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-17  2:17 bug#59328: 29.0.50; `seq-keep' implementation only valid for lists Michael Heerdegen
2022-11-19 13:12 ` Eli Zaretskii
2022-11-24 13:04   ` Michael Heerdegen
2022-11-24 14:19     ` Eli Zaretskii
2022-11-24 14:26       ` Michael Heerdegen
2022-11-24 15:00         ` Eli Zaretskii
2022-11-24 15:09           ` Michael Heerdegen
2022-11-24 15:15             ` Eli Zaretskii
2022-11-24 15:25               ` Michael Heerdegen
2022-11-24 17:02                 ` Eli Zaretskii
2022-11-25  9:47                   ` Michael Heerdegen
2022-11-25 11:34                     ` Stefan Kangas
2022-11-25 11:55                     ` Eli Zaretskii

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).