From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Nicolas Petton Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] sequence manipulation functions Date: Mon, 10 Nov 2014 23:28:37 +0100 Message-ID: <87k332u1uy.fsf@gmail.com> References: <87oasmmwzt.fsf@gmail.com> <87bnolslph.fsf@gmail.com> <87zjc2dic0.fsf@gmail.com> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1415658542 9807 80.91.229.3 (10 Nov 2014 22:29:02 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 10 Nov 2014 22:29:02 +0000 (UTC) Cc: Emacs developers To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Nov 10 23:28:56 2014 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1XnxSC-0002Jp-Dr for ged-emacs-devel@m.gmane.org; Mon, 10 Nov 2014 23:28:56 +0100 Original-Received: from localhost ([::1]:45362 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XnxSC-0006xZ-0H for ged-emacs-devel@m.gmane.org; Mon, 10 Nov 2014 17:28:56 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:59666) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XnxS4-0006xE-Pc for emacs-devel@gnu.org; Mon, 10 Nov 2014 17:28:53 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XnxS0-0000h5-2U for emacs-devel@gnu.org; Mon, 10 Nov 2014 17:28:48 -0500 Original-Received: from mail-la0-x22e.google.com ([2a00:1450:4010:c03::22e]:62973) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XnxRz-0000gs-EF for emacs-devel@gnu.org; Mon, 10 Nov 2014 17:28:43 -0500 Original-Received: by mail-la0-f46.google.com with SMTP id gm9so8644206lab.33 for ; Mon, 10 Nov 2014 14:28:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=references:user-agent:from:to:cc:subject:in-reply-to:date :message-id:mime-version:content-type; bh=heierxffsfNQ9t/f2+0bmfTxm/m2pH9vJvycSPn6V+w=; b=hJ1OjcxOvu1w+5TSpiiVPqbVqyvLyxWFROPW0pi0TpRT6c3Eyrzr4RVuk7GpBZK9q6 LbfJQpQkavLq0Kor1SI2L8uAek6r6foPl6W4Qe0vJ7k2Q5IT8FXJRtXLb7iJSDXMhCXY 0cjBxwc/GFQOxnWclnDsWHGKM5dvzdukm6/NVR/rItKTLF2xcgHvZMPBXxeycpuz5SUA xu2PLhrYZzrkfaSRfZmW12bN9nzXmDch5b+1vokLuAhoeO8TrSGdRKXDG3H3owi9jwgA nMq/6iVIK/z47eW+oClojpjSFLMM2Cd/5IJb/EqYTXcD1MmQ/jzsOMrJWz0b02hjttYr ziFQ== X-Received: by 10.112.173.100 with SMTP id bj4mr5952780lbc.78.1415658520458; Mon, 10 Nov 2014 14:28:40 -0800 (PST) Original-Received: from blueberry (c213-89-134-104.bredband.comhem.se. [213.89.134.104]) by mx.google.com with ESMTPSA id xn9sm5570367lbb.25.2014.11.10.14.28.38 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 10 Nov 2014 14:28:39 -0800 (PST) User-agent: mu4e 0.9.9.6pre3; emacs 24.3.1 In-reply-to: X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c03::22e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:176721 Archived-At: Thank you Stefan for your review. I will submit another patch shortly. Nico Stefan Monnier 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