Re: [PATCH] sequence manipulation functions

[Stefan Monnier <monnier@iro.umontreal.ca>]

> +;; Sequence manipulation functions

As noticed in the previous review, this just repeats what's already on
the first line, so that's not useful.  OTOH An important info would be
to define what we mean by "sequence" here, more specifically which kinds
of sequences are supported.

Also we could document here the conventions that we tried to follow.
E.g. what is the argument ordering.

> +(require 'pcase)

Why?  The `pcase' macro is autoloaded, so you should only need to
(require 'pcase) if you want top use macros like pcase-dolist.

> +(defun seq-filter (pred seq)
> +  "Return a list filtering with PRED all elements of SEQ."

Doesn't say if PRED returning nil means "keep" or "throw away".

> +  (delq nil (mapcar (lambda (elt)
> +                      (and (funcall pred elt) elt))
> +                    seq)))

This will filter out the nil elements, regardless of `pred'.

> +(defun seq-reduce (function seq &optional initial-value)
> +  "Reduce FUNCTION across SEQ, starting with INITIAL-VALUE if not nil."

You don't say how FUNCTION will be called.

> +  (or (listp seq) (setq seq (append seq '())))
> +  (let ((acc (or initial-value (if (seq-empty-p seq)
> +                                   (funcall function)
> +                                 (car seq)))))

If you want an initial value of nil but your function can only be called
with 2 arguments, you're screwed :-(
Would it be a real problem if we simply made `initial-value' an
mandatory argument?

> +(defun seq-some-p (pred seq)
> +  "Return any element for which PRED is true in SEQ, nil otherwise."
                                           ^^^^
Usually we say "non-nil".

> +  (catch 'seq-some-p-break

Since this tag is internal, I'd use a "seq--" prefix.  I think
`seq--break' should be sufficient.  Of course you could also just use
cl-block and cl-return and circumvent the question.

> +(defun seq-every-p (pred seq)
> +  "Return t if (PRED item) is non-nil for all elements of the sequence SEQ."
             ^
          non-nil

> +(defun seq-empty-p (seq)
> +  "Return t if the sequence SEQ is empty, nil otherwise."
             ^
          non-nil

> +(defun seq-sort (seq pred)
> +  "Return a sorted list of the elements of SEQ compared using PRED."

I wonder if that's really the more useful behavior, compared to the
"inplace" sorting of `sort', or compared to the alternative is always
returning a new sequence, but of the same type as the `seq' argument.

> +(defun seq-concatenate (type &rest seqs)
> +  "Concatenate, into a sequence of type TYPE, the argument SEQS.
> +\n(fn TYPE SEQUENCE...)"

You need to document the values that TYPE can take.  Maybe you can
simply refer to `type-of'.

> +(defalias 'seq-copy #'copy-sequence)
> +(defalias 'seq-elt #'elt)
> +(defalias 'seq-length #'length)

I think mapc would make a lot of sense, and I guess mapcar as well.
Not sure if we should name them `seq-mapc' and `seq-mapcar' or
something else.

> +(load "emacs-lisp/sequences")

I think I'd rather not preload it for now and let people use (require
'seq) for that.  There's 30 years of accumulated Elisp code and we're
not going to switch them to use a new naming scheme overnight.  It might
even be that people will simply not like to have to add "seq-" in their
code (I know they complained about adding "cl-"), so I'd start by simply
providing the library and when its popularity grows (and/or is being
used by preloaded code) we can then add it to loadup.


        Stefan