A few thoughts on seq.el

A few thoughts on seq.el

[Tino Calancha]

Dear Nico,

I realize a delightful minimalist design choice in this lib:
to keep just the minimum set of funtions providing same funcionality.
For instance, seq.el has neither `seq-notevery' nor `seq-notany'
which can be easily obtained as negation of `seq-every-p' and `seq-some'.

Another example, instead of defining two functions per each operation
,as CL does (*) for several functions,

I) func (seq, elt), one receiving args (SEQ ELT).
II) func-if (pred, seq), other receiving args (PRED SEQ).

seq.el seems to prefer introducing just the more general funcs in II) 
(**).

There are two functions that escape from this logic.
A) seq-position
   I suggest to define this function so that it receives args as II),
   like other functions in the file.
   With the current definition, it is not possible to recover
   the useful behaviour in II) (***).

B) seq-contains
   Once you add this in the lib, the pair (seq-contains, seq-find)
   is analog to CL (find, find-if).  This breaks the minimalist design
   choice mentioned above.

   I believe `seq-contains' was introduced for convenience:
   it is used elsewhere in seq.el and map.el.
   For consistency with the rest of the file, i would expect
   not having defined `seq-contains', though.

C) Does something like this have any sense? ; possible with different name
(map-delete-if PRED MAP)
"Delete all keys from MAP satisfying (PRED key val) and return MAP.
If MAP is an array, store nil at the index of each deleted key.

MAP can be a list, hash-table or array."

;; As `map-remove' but returning the same type as (type-of MAP).

D) If the only dependence on 'cl-lib in seq.el is `cl-subseq',
    Would be possible to include a func `seq-replace'
    (stolen from `cl-replace') and then define `seq-subseq',
    a stolen version of `cl-subseq' as well?

    That would remove the 'cl-lib dependence, one of the
    arguments to not load 'seq at Emacs start up, AFAIK.
    Does it have any sense?

----------
(*)
In fact, CL define func-if-not, as well.

(**)
We can get I) behaviour from II) with ease.

(***)
- we can get CL `position' from `position-if'.
- we cannot get CL `position-if' from `position'.


Regards,
Tino


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 1fc3a02cbc275e98876efbc9a85e65891c4937c3 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Sun, 31 Jul 2016 13:39:01 +0900
Subject: [PATCH 1/2] seq-position: accepts 2 args, PRED and SEQUENCE

* lisp/emacs-lisp/seq.el (seq-position): Function accepts 2 arg
PRED and SEQUENCE; return the index of the first element in
SEQUENCE for which (PRED element) is non-nil.
All callers updated.
---
  lisp/emacs-lisp/seq.el            |  9 ++++-----
  lisp/emacs-lisp/subr-x.el         |  2 +-
  lisp/ibuffer.el                   |  2 +-
  test/lisp/emacs-lisp/seq-tests.el | 10 +++++-----
  4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/lisp/emacs-lisp/seq.el b/lisp/emacs-lisp/seq.el
index e5004f8..9a97f55 100644
--- a/lisp/emacs-lisp/seq.el
+++ b/lisp/emacs-lisp/seq.el
@@ -353,13 +353,12 @@ seq-sort-by
                  e))
              sequence))

-(cl-defgeneric seq-position (sequence elt &optional testfn)
-  "Return the index of the first element in SEQUENCE that is equal to 
ELT.
-Equality is defined by TESTFN if non-nil or by `equal' if nil."
+(cl-defgeneric seq-position (pred sequence)
+"Return the index of the first element in SEQUENCE for which (PRED 
element) is non-nil."
    (let ((index 0))
      (catch 'seq--break
-      (seq-doseq (e sequence)
-        (when (funcall (or testfn #'equal) e elt)
+      (seq-doseq (elt sequence)
+        (when (funcall pred elt)
            (throw 'seq--break index))
          (setq index (1+ index)))
        nil)))
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 173cd11..7d067d2 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -233,7 +233,7 @@ read-multiple-choice
             (mapconcat
              (lambda (elem)
                (let* ((name (cadr elem))
-                     (pos (seq-position name (car elem)))
+                     (pos (seq-position (lambda (e) (equal e (car elem))) 
name))
                       (altered-name
                        (cond
                         ;; Not in the name string.
diff --git a/lisp/ibuffer.el b/lisp/ibuffer.el
index 8e24629..4ff753c 100644
--- a/lisp/ibuffer.el
+++ b/lisp/ibuffer.el
@@ -1813,7 +1813,7 @@ name
    (let ((string (propertize (buffer-name)
                              'font-lock-face
                              (ibuffer-buffer-name-face buffer mark))))
-    (if (not (seq-position string ?\n))
+    (if (not (seq-position (lambda (e) (char-equal e ?\n)) string))
          string
        (replace-regexp-in-string
         "\n" (propertize "^J" 'font-lock-face 'escape-glyph) string))))
diff --git a/test/lisp/emacs-lisp/seq-tests.el 
b/test/lisp/emacs-lisp/seq-tests.el
index c2065c6..8302cf4 100644
--- a/test/lisp/emacs-lisp/seq-tests.el
+++ b/test/lisp/emacs-lisp/seq-tests.el
@@ -359,12 +359,12 @@ test-sequences-oddp

  (ert-deftest test-seq-position ()
    (with-test-sequences (seq '(2 4 6))
-    (should (null (seq-position seq 1)))
-    (should (= (seq-position seq 4) 1)))
+    (should-not (seq-position (lambda (e) (= e 1)) seq))
+    (should (= (seq-position (lambda (e) (= e 4)) seq) 1)))
    (let ((seq '(a b c)))
-    (should (null (seq-position seq 'd #'eq)))
-    (should (= (seq-position seq 'a #'eq) 0))
-    (should (null (seq-position seq (make-symbol "a") #'eq)))))
+    (should-not (seq-position (lambda (e) (eq e 'd)) seq))
+    (should (= (seq-position (lambda (e) (eq e 'a)) seq) 0))
+    (should-not (seq-position (lambda (e) (eq e (make-symbol "a"))) 
seq))))

  (ert-deftest test-seq-sort-by ()
    (let ((seq ["x" "xx" "xxx"]))
-- 
2.8.1

From bd7284cdd09908caa5ef7c89b669b8a0be0a20d8 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Sun, 31 Jul 2016 14:22:20 +0900
Subject: [PATCH 2/2] Delete seq-contains

* lisp/emacs-lisp/seq.el (seq-contains): Delete this function.
Updated all callers: callers use 'seq-find' instead.
---
  lisp/emacs-lisp/map.el            |  4 +++-
  lisp/emacs-lisp/seq.el            | 12 +++++++++---
  test/lisp/emacs-lisp/seq-tests.el | 16 ++++++----------
  3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/lisp/emacs-lisp/map.el b/lisp/emacs-lisp/map.el
index 98a8871..cce6bd6 100644
--- a/lisp/emacs-lisp/map.el
+++ b/lisp/emacs-lisp/map.el
@@ -264,7 +264,9 @@ map-contains-key
  Equality is defined by TESTFN if non-nil or by `equal' if nil.

  MAP can be a list, hash-table or array."
-  (seq-contains (map-keys map) key testfn))
+  (seq-find (lambda (e)
+              (funcall (or testfn #'equal) e key))
+            (map-keys map)))

  (defun map-some (pred map)
    "Return a non-nil if (PRED key val) is non-nil for any key/value pair 
in MAP.
diff --git a/lisp/emacs-lisp/seq.el b/lisp/emacs-lisp/seq.el
index 9a97f55..5082449 100644
--- a/lisp/emacs-lisp/seq.el
+++ b/lisp/emacs-lisp/seq.el
@@ -368,7 +368,9 @@ seq-sort-by
  TESTFN is used to compare elements, or `equal' if TESTFN is nil."
    (let ((result '()))
      (seq-doseq (elt sequence)
-      (unless (seq-contains result elt testfn)
+      (unless (seq-find
+               (lambda (e) (funcall (or testfn #'equal) e elt))
+               result)
          (setq result (cons elt result))))
      (nreverse result)))

@@ -393,7 +395,9 @@ seq-sort-by
    "Return a list of the elements that appear in both SEQUENCE1 and 
SEQUENCE2.
  Equality is defined by TESTFN if non-nil or by `equal' if nil."
    (seq-reduce (lambda (acc elt)
-                (if (seq-contains sequence2 elt testfn)
+                (if (seq-find (lambda (e)
+                                (funcall (or testfn #'equal) e elt))
+                              sequence2)
                      (cons elt acc)
                    acc))
                (seq-reverse sequence1)
@@ -403,7 +407,9 @@ 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-find (lambda (e)
+                                     (funcall (or testfn #'equal) e elt))
+                                   sequence2))
                      (cons elt acc)
                    acc))
                (seq-reverse sequence1)
diff --git a/test/lisp/emacs-lisp/seq-tests.el 
b/test/lisp/emacs-lisp/seq-tests.el
index 8302cf4..8cde9ac 100644
--- a/test/lisp/emacs-lisp/seq-tests.el
+++ b/test/lisp/emacs-lisp/seq-tests.el
@@ -171,19 +171,15 @@ test-sequences-oddp
    (should-not (seq-find #'null '(1 nil 2)))
    (should-not (seq-find #'null '(1 nil 2) t))
    (should-not (seq-find #'null '(1 2 3)))
-  (should (seq-find #'null '(1 2 3) 'sentinel)))
-
-(ert-deftest test-seq-contains ()
+  (should (seq-find #'null '(1 2 3) 'sentinel))
    (with-test-sequences (seq '(3 4 5 6))
-    (should (seq-contains seq 3))
-    (should-not (seq-contains seq 7)))
+    (should (seq-find (lambda (e) (equal e 3)) seq))
+    (should-not (seq-find (lambda (e) (equal e 7)) seq)))
    (with-test-sequences (seq '())
-    (should-not (seq-contains seq 3))
-    (should-not (seq-contains seq nil))))
-
-(ert-deftest test-seq-contains-should-return-the-elt ()
+    (should-not (seq-find (lambda (e) (equal e 3)) seq))
+    (should-not (seq-find (lambda (e) (equal e nil)) seq)))
    (with-test-sequences (seq '(3 4 5 6))
-    (should (= 5 (seq-contains seq 5)))))
+    (should (= 5 (seq-find (lambda (e) (equal e 5)) seq)))))

  (ert-deftest test-seq-every-p ()
    (with-test-sequences (seq '(43 54 22 1))
-- 
2.8.1

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

In GNU Emacs 25.1.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.20.6)
  of 2016-07-31 built on calancha-pc
Repository revision: 7d58b02f363ab02961faa950d1ba727df96f2f19

Re: A few thoughts on seq.el

[Nicolas Petton]

[-- Attachment #1: Type: text/plain, Size: 3111 bytes --]

Tino Calancha <tino.calancha@gmail.com> writes:

> Dear Nico,

Hi Tino,

> I realize a delightful minimalist design choice in this lib:
> to keep just the minimum set of funtions providing same funcionality.
> For instance, seq.el has neither `seq-notevery' nor `seq-notany'
> which can be easily obtained as negation of `seq-every-p' and
> `seq-some'.

Glad you like it!

> Another example, instead of defining two functions per each operation
> ,as CL does (*) for several functions,
>
> I) func (seq, elt), one receiving args (SEQ ELT).
> II) func-if (pred, seq), other receiving args (PRED SEQ).
>
> seq.el seems to prefer introducing just the more general funcs in II) 
> (**).

Indeed, with some exceptions (that you already caught).

> There are two functions that escape from this logic.
> A) seq-position
>    I suggest to define this function so that it receives args as II),
>    like other functions in the file.
>    With the current definition, it is not possible to recover
>    the useful behaviour in II) (***).

I think this is a valid exception to the (pred, seq) signature of seq.el
methods.  The reason is that it would be counter-intuitive to retrieve
the position of an element in a sequence, which is really what
`seq-position' is about.

However, I could introduce another function that would take a predicate,
and `seq-position' could reuse it.  But I don't want to get rid of
`seq-position' as it exists today, I think it's too useful.

> B) seq-contains
>    Once you add this in the lib, the pair (seq-contains, seq-find)
>    is analog to CL (find, find-if).  This breaks the minimalist design
>    choice mentioned above.

Not exactly.  `seq-contains' might actually be a bit misleading.  It is
in essence a predicate, but it is not named `seq-contains-p' because it
returns the element if found.  Does that make more sense?

>    I believe `seq-contains' was introduced for convenience:
>    it is used elsewhere in seq.el and map.el.

It is indeed very convenient (as a predicate-ish function), and I think
that seq.el wouldn't be complete without it.

That said, renaming it `seq-contains-p' might clear things up, but OTOH
the function would lose part of its usefulness.

>    For consistency with the rest of the file, i would expect
>    not having defined `seq-contains', though.
>
> D) If the only dependence on 'cl-lib in seq.el is `cl-subseq',

There are all the generic functions as well which are defined in cl-lib.

>     Would be possible to include a func `seq-replace'
>     (stolen from `cl-replace')

I want to avoid side-effects on sequences as much as possible, so I
would vote against it.

>     That would remove the 'cl-lib dependence, one of the
>     arguments to not load 'seq at Emacs start up, AFAIK.
>     Does it have any sense?

Unfortunately, cl-lib will always be required for the generic functions.
That said, what I would really like is to see `cl-defgeneric' and
`cl-defmethod' be renamed and moved to Elisp core, they are so useful
and powerful.

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: A few thoughts on seq.el

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 611 bytes --]

On 2016-07-31 13:36, Nicolas Petton wrote:
>> >     That would remove the 'cl-lib dependence, one of the
>> >     arguments to not load 'seq at Emacs start up, AFAIK.
>> >     Does it have any sense?
>
> Unfortunately, cl-lib will always be required for the generic functions.
> That said, what I would really like is to see `cl-defgeneric' and
> `cl-defmethod' be renamed and moved to Elisp core, they are so useful
> and powerful.

Hmm. I don't think cl-defgeneric and cl-defmethods are in cl-lib, actually: they are part of cl-generic, which is already loaded at startup :)

Cheers,
Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: A few thoughts on seq.el

[Nicolas Petton]

[-- Attachment #1: Type: text/plain, Size: 243 bytes --]

Clément Pit--Claudel <clement.pit@gmail.com> writes:

> Hmm. I don't think cl-defgeneric and cl-defmethods are in cl-lib,
> actually: they are part of cl-generic, which is already loaded at
> startup :)

You're right!

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: A few thoughts on seq.el

[Nicolas Petton]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

Tino Calancha <tino.calancha@gmail.com> writes:

> D) If the only dependence on 'cl-lib in seq.el is `cl-subseq',
>     Would be possible to include a func `seq-replace'
>     (stolen from `cl-replace') and then define `seq-subseq',
>     a stolen version of `cl-subseq' as well?
>
>     That would remove the 'cl-lib dependence, one of the
>     arguments to not load 'seq at Emacs start up, AFAIK.
>     Does it have any sense?

I take back what I previously said, I think it could make sense, even if
I generally don't like copy/pasting code.

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: A few thoughts on seq.el

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 256 bytes --]

On 2016-08-01 04:48, Nicolas Petton wrote:
> I take back what I previously said, I think it could make sense, even if
> I generally don't like copy/pasting code.

Well, we could reverse the dependency and make cl-lib depend on seq.el :)

Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: A few thoughts on seq.el

[Nicolas Petton]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

Clément Pit--Claudel <clement.pit@gmail.com> writes:

> Well, we could reverse the dependency and make cl-lib depend on seq.el
> :)

That's how I did it first, and for some reason I had to reverse the
dependency, but I forgot the reason, I should have a look at the git
log.

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: A few thoughts on seq.el

[Michael Heerdegen]

Nicolas Petton <nicolas@petton.fr> writes:

> I take back what I previously said, I think it could make sense, even
> if I generally don't like copy/pasting code.

FWIW, there is also room for improvement in the implementation: If a
list L has, say, 100000 elements,

   (cl-subseq l 1 3)

will be very slow because it always calculates the length of the first
argument L.  Maybe we can do it better in seq.el.


Michael.

Re: A few thoughts on seq.el

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> list L has, say, 100000 elements,
>
>    (cl-subseq l 1 3)
>
> will be very slow because it always calculates the length of the first
> argument L.

Eh, no, that must have been another function, or it has been fixed.
What I said is not correct, the function calculates the list length only
when necessary (negative indexes).


Michael.

Re: A few thoughts on seq.el

[Tino Calancha]

On Sun, 31 Jul 2016, Nicolas Petton wrote:

> However, I could introduce another function that would take a predicate,
> and `seq-position' could reuse it.  But I don't want to get rid of
> `seq-position' as it exists today, I think it's too useful.

It would be great having such new function.
I have some code which use `cl-position-if'.
With the inclusion of this new func in 'seq i could
switch easily to 'seq instead of 'cl-lib if i wish.