bug#50965: Allow nil third argument for mapconcat

bug#50965: Allow nil third argument for mapconcat

[Stefan Kangas]

Severity: wishlist

Please consider allowing an empty third argument for mapconcat that
means the empty string, i.e.:

    (mapconcat FUNCTION SEQUENCE SEPARATOR)

Becomes:

    (mapconcat FUNCTION SEQUENCE &optional SEPARATOR)

bug#50965: Allow nil third argument for mapconcat

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Please consider allowing an empty third argument for mapconcat that
> means the empty string, i.e.:
>
>     (mapconcat FUNCTION SEQUENCE SEPARATOR)
>
> Becomes:
>
>     (mapconcat FUNCTION SEQUENCE &optional SEPARATOR)

I think that makes sense -- SEPARATOR is optional in the related

(string-join strings &optional separator)

function.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50965: Allow nil third argument for mapconcat

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I think that makes sense -- SEPARATOR is optional in the related
>
> (string-join strings &optional separator)
>
> function.

Aha, so an explicit nil third argument is already treated as the empty string:

    (mapconcat #'identity '("foo" "bar") nil)

This is the case also in 27.2.

bug#50965: Allow nil third argument for mapconcat

[Stefan Kangas]

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

tags 50965 + patch
thanks

Stefan Kangas <stefan@marxist.se> writes:

> Aha, so an explicit nil third argument is already treated as the empty string:
>
>     (mapconcat #'identity '("foo" "bar") nil)

How does the attached patch look?  I'm assuming this should go to
master at this point.

[-- Attachment #2: 0001-Make-third-mapconcat-argument-optional.patch --]
[-- Type: application/octet-stream, Size: 3358 bytes --]

From ac2f687c45c6a298fbc1cbfd6b7b21f5f17c3d3c Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Sun, 3 Oct 2021 00:41:13 +0200
Subject: [PATCH] Make third mapconcat argument optional

* src/fns.c (Fmapconcat): Make 'separator' argument optional.  (Bug#50965)
* test/src/fns-tests.el (fns-tests-mapconcat): New test.
* lisp/emacs-lisp/subr-x.el (string-join): Document 'separator'
argument better.
---
 etc/NEWS                  | 3 +++
 lisp/emacs-lisp/subr-x.el | 4 +++-
 src/fns.c                 | 8 ++++++--
 test/src/fns-tests.el     | 7 +++++++
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 8c22230daf..7be043fc99 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -80,6 +80,9 @@ Emacs buffers, like indentation and the like.  The new ert function
 \f
 * Lisp Changes in Emacs 29.1
 
+** Third 'mapconcat' argument 'separator' is now optional.
+An explicit nil always meant the empty string, now it can be left out.
+
 \f
 * Changes in Emacs 29.1 on Non-Free Operating Systems
 
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 91ebbf9fb9..2bdbdaab74 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -208,7 +208,9 @@ string-empty-p
   (string= string ""))
 
 (defsubst string-join (strings &optional separator)
-  "Join all STRINGS using SEPARATOR."
+  "Join all STRINGS using SEPARATOR.
+Optional argument SEPARATOR must be a string, a vector, or a list of
+characters; nil is the empty string."
   (mapconcat #'identity strings separator))
 
 (define-obsolete-function-alias 'string-reverse 'reverse "25.1")
diff --git a/src/fns.c b/src/fns.c
index a72e41aee5..f77b6009bd 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2852,12 +2852,16 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
   return leni;
 }
 
-DEFUN ("mapconcat", Fmapconcat, Smapconcat, 3, 3, 0,
+DEFUN ("mapconcat", Fmapconcat, Smapconcat, 2, 3, 0,
        doc: /* Apply FUNCTION to each element of SEQUENCE, and concat the results as strings.
 In between each pair of results, stick in SEPARATOR.  Thus, " " as
   SEPARATOR results in spaces between the values returned by FUNCTION.
+
 SEQUENCE may be a list, a vector, a bool-vector, or a string.
-SEPARATOR must be a string, a vector, or a list of characters.
+
+Optional argument SEPARATOR must be a string, a vector, or a list of
+characters; nil is the empty string.
+
 FUNCTION must be a function of one argument, and must return a value
   that is a sequence of characters: either a string, or a vector or
   list of numbers that are valid character codepoints.  */)
diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
index 5759457209..a771790b68 100644
--- a/test/src/fns-tests.el
+++ b/test/src/fns-tests.el
@@ -430,6 +430,13 @@ fns-tests-hash-buffer
                    (buffer-hash))
                  (sha1 "foo"))))
 
+(ert-deftest fns-tests-mapconcat ()
+  (should (string= (mapconcat #'identity '() "_") ""))
+  (should (string= (mapconcat #'identity '("A") "_") "A"))
+  (should (string= (mapconcat #'identity '("A" "B") "_") "A_B"))
+  (should (string= (mapconcat #'identity '("A" "B" "C") "_") "A_B_C"))
+  (should (string= (mapconcat #'identity '("a" "b")) "ab")))
+
 (ert-deftest fns-tests-mapcan ()
   (should-error (mapcan))
   (should-error (mapcan #'identity))
-- 
2.33.0

bug#50965: Allow nil third argument for mapconcat

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sun, 3 Oct 2021 01:03:54 +0200
> Cc: 50965@debbugs.gnu.org
> 
> +Optional argument SEPARATOR must be a string, a vector, or a list of
> +characters; nil is the empty string."

nil is not a string, so saying that is not the best wording.  I
suggest "nil stands for an empty string." instead.

> +Optional argument SEPARATOR must be a string, a vector, or a list of
> +characters; nil is the empty string.

Likewise.

> +(ert-deftest fns-tests-mapconcat ()
> +  (should (string= (mapconcat #'identity '() "_") ""))
> +  (should (string= (mapconcat #'identity '("A") "_") "A"))
> +  (should (string= (mapconcat #'identity '("A" "B") "_") "A_B"))
> +  (should (string= (mapconcat #'identity '("A" "B" "C") "_") "A_B_C"))
> +  (should (string= (mapconcat #'identity '("a" "b")) "ab")))

Please be sure to test with vectors, bool-vectors, list of characters,
and non-ASCII strings as well.

> I'm assuming this should go to master at this point.

Yes, thanks.

bug#50965: Allow nil third argument for mapconcat

[Stefan Kangas]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> +Optional argument SEPARATOR must be a string, a vector, or a list of
>> +characters; nil is the empty string."
>
> nil is not a string, so saying that is not the best wording.  I
> suggest "nil stands for an empty string." instead.

Yes, that's better.  Fixed.

>> +(ert-deftest fns-tests-mapconcat ()
>> +  (should (string= (mapconcat #'identity '() "_") ""))
>> +  (should (string= (mapconcat #'identity '("A") "_") "A"))
>> +  (should (string= (mapconcat #'identity '("A" "B") "_") "A_B"))
>> +  (should (string= (mapconcat #'identity '("A" "B" "C") "_") "A_B_C"))
>> +  (should (string= (mapconcat #'identity '("a" "b")) "ab")))
>
> Please be sure to test with vectors, bool-vectors, list of characters,
> and non-ASCII strings as well.

I've added such tests.  Please check especially the bool-vector case,
as I was unsure about what exactly to test for.  I put this:

    (should (string= (mapconcat #'identity [nil nil] "") ""))
    (should-error (mapconcat #'identity [nil nil t])
                  :type 'wrong-type-argument)

>> I'm assuming this should go to master at this point.
>
> Yes, thanks.

New patch attached.

[-- Attachment #2: 0001-Make-mapconcat-argument-separator-optional.patch --]
[-- Type: text/x-diff, Size: 5918 bytes --]

From 7646c687a899f8bba7e71c6f5f59f4262bb7d592 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Sun, 3 Oct 2021 19:24:31 +0200
Subject: [PATCH] Make 'mapconcat' argument 'separator' optional

* src/fns.c (Fmapconcat): Make third 'separator' argument
optional.  (Bug#50965)
* doc/lispref/functions.texi (Mapping Functions): Update
documentation for above change.
* test/src/fns-tests.el (fns-tests-mapconcat): New test.

* doc/misc/cl.texi (Obsolete Setf Customization): Don't use third
mapconcat argument in example.
* lisp/emacs-lisp/subr-x.el (string-join): Doc fix.
---
 doc/lispref/functions.texi |  9 ++++-----
 doc/misc/cl.texi           |  2 +-
 etc/NEWS                   |  4 ++++
 lisp/emacs-lisp/subr-x.el  |  4 +++-
 src/fns.c                  |  8 ++++++--
 test/src/fns-tests.el      | 17 +++++++++++++++++
 6 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi
index c856557c3c..3163300184 100644
--- a/doc/lispref/functions.texi
+++ b/doc/lispref/functions.texi
@@ -961,14 +961,14 @@ Mapping Functions
 into a list.  @code{mapc} always returns @var{sequence}.
 @end defun
 
-@defun mapconcat function sequence separator
+@defun mapconcat function sequence &optional separator
 @code{mapconcat} applies @var{function} to each element of
 @var{sequence}; the results, which must be sequences of characters
 (strings, vectors, or lists), are concatenated into a single string
 return value.  Between each pair of result sequences, @code{mapconcat}
 inserts the characters from @var{separator}, which also must be a
-string, or a vector or list of characters.  @xref{Sequences Arrays
-Vectors}.
+string, or a vector or list of characters; a @code{nil} value is
+treated as the empty string.  @xref{Sequences Arrays Vectors}.
 
 The argument @var{function} must be a function that can take one
 argument and returns a sequence of characters: a string, a vector, or
@@ -986,8 +986,7 @@ Mapping Functions
 
 @group
 (mapconcat (lambda (x) (format "%c" (1+ x)))
-           "HAL-8000"
-           "")
+           "HAL-8000")
      @result{} "IBM.9111"
 @end group
 @end example
diff --git a/doc/misc/cl.texi b/doc/misc/cl.texi
index a6c3c32c0e..f31dedae88 100644
--- a/doc/misc/cl.texi
+++ b/doc/misc/cl.texi
@@ -5024,7 +5024,7 @@ Obsolete Setf Customization
 @ignore
 (defmacro concatf (place &rest args)
   (gv-letplace (getter setter) place
-    (macroexp-let2 nil v (mapconcat 'identity args "")
+    (macroexp-let2 nil v (mapconcat 'identity args)
       (funcall setter `(concat ,getter ,v)))))
 @end ignore
 @end defmac
diff --git a/etc/NEWS b/etc/NEWS
index 20ed20308e..afd335163b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -80,6 +80,10 @@ Emacs buffers, like indentation and the like.  The new ert function
 \f
 * Lisp Changes in Emacs 29.1
 
++++
+** Third 'mapconcat' argument 'separator' is now optional.
+An explicit nil always meant the empty string, now it can be left out.
+
 ---
 ** Themes can now be made obsolete.
 Using 'make-obsolete' on a theme is now supported.  This will make
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 91ebbf9fb9..7d1b6d85ed 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -208,7 +208,9 @@ string-empty-p
   (string= string ""))
 
 (defsubst string-join (strings &optional separator)
-  "Join all STRINGS using SEPARATOR."
+  "Join all STRINGS using SEPARATOR.
+Optional argument SEPARATOR must be a string, a vector, or a list of
+characters; nil stands for the empty string."
   (mapconcat #'identity strings separator))
 
 (define-obsolete-function-alias 'string-reverse 'reverse "25.1")
diff --git a/src/fns.c b/src/fns.c
index a72e41aee5..61182852ba 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2852,12 +2852,16 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
   return leni;
 }
 
-DEFUN ("mapconcat", Fmapconcat, Smapconcat, 3, 3, 0,
+DEFUN ("mapconcat", Fmapconcat, Smapconcat, 2, 3, 0,
        doc: /* Apply FUNCTION to each element of SEQUENCE, and concat the results as strings.
 In between each pair of results, stick in SEPARATOR.  Thus, " " as
   SEPARATOR results in spaces between the values returned by FUNCTION.
+
 SEQUENCE may be a list, a vector, a bool-vector, or a string.
-SEPARATOR must be a string, a vector, or a list of characters.
+
+Optional argument SEPARATOR must be a string, a vector, or a list of
+characters; nil stands for the empty string.
+
 FUNCTION must be a function of one argument, and must return a value
   that is a sequence of characters: either a string, or a vector or
   list of numbers that are valid character codepoints.  */)
diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
index 5759457209..2d641cc311 100644
--- a/test/src/fns-tests.el
+++ b/test/src/fns-tests.el
@@ -430,6 +430,23 @@ fns-tests-hash-buffer
                    (buffer-hash))
                  (sha1 "foo"))))
 
+(ert-deftest fns-tests-mapconcat ()
+  (should (string= (mapconcat #'identity '()) ""))
+  (should (string= (mapconcat #'identity '("a" "b")) "ab"))
+  (should (string= (mapconcat #'identity '() "_") ""))
+  (should (string= (mapconcat #'identity '("A") "_") "A"))
+  (should (string= (mapconcat #'identity '("A" "B") "_") "A_B"))
+  (should (string= (mapconcat #'identity '("A" "B" "C") "_") "A_B_C"))
+  ;; non-ASCII strings
+  (should (string= (mapconcat #'identity '("Ä" "ø" "☭" "தமிழ்") "_漢字_")
+                   "Ä_漢字_ø_漢字_☭_漢字_தமிழ்"))
+  ;; vector
+  (should (string= (mapconcat #'identity ["a" "b"] "") "ab"))
+  ;; bool-vector
+  (should (string= (mapconcat #'identity [nil nil] "") ""))
+  (should-error (mapconcat #'identity [nil nil t])
+                :type 'wrong-type-argument))
+
 (ert-deftest fns-tests-mapcan ()
   (should-error (mapcan))
   (should-error (mapcan #'identity))
-- 
2.30.2

bug#50965: Allow nil third argument for mapconcat

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sun, 3 Oct 2021 18:34:31 +0000
> Cc: larsi@gnus.org, 50965@debbugs.gnu.org
> 
> New patch attached.

LGTM, thanks.

bug#50965: Allow nil third argument for mapconcat

[Stefan Kangas]

tags 50965 fixed
close 50965 29.1
thanks

Eli Zaretskii <eliz@gnu.org> writes:

> LGTM, thanks.

Thanks, pushed to master as commit d652efcd08.