Re: [PATCH] sequence manipulation functions

[Nicolas Petton <petton.nicolas@gmail.com>]

Thank you Stefan for your review. I will submit another patch shortly.

Nico


Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +;; 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

-- 
Nicolas Petton
http://nicolas-petton.fr