unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* [PATCH] Unit tests and lexical-binding for delim-col.el
@ 2019-05-07 20:56 Stefan Kangas
  2019-05-07 22:44 ` Basil L. Contovounesios
  0 siblings, 1 reply; 12+ messages in thread
From: Stefan Kangas @ 2019-05-07 20:56 UTC (permalink / raw)
  To: emacs-devel


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

Hi all,

I was looking into some area where I could contribute and ended up writing
unit
tests for `delim-col.el' and adding the lexical-binding header.

I wasn't inclined to convert the code in delim-col.el to actually take
advantage
of lexical bindings, but it compiles and runs fine with just naively adding
the
header.

Please let me know what you think or if I've made any obvious mistakes.

Thanks,
Stefan Kangas

PS. Since this patch is longer than 15 lines, I have initiated the copyright
assignment process.  It wasn't obvious if this should be done before or
after
mailing this list.

[-- Attachment #1.2: Type: text/html, Size: 779 bytes --]

[-- Attachment #2: 0001-lisp-delim-col.el-Use-lexical-binding.patch --]
[-- Type: text/x-patch, Size: 8508 bytes --]

From f11bd037478aa64b83ffe97a925357cddc886316 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 5 May 2019 15:48:57 +0200
Subject: [PATCH] * lisp/delim-col.el: Use lexical-binding

* test/lisp/delim-col-tests.el (delim-col-tests-delimit-colummns-before-after)
(delim-col-tests-delimit-columns)
(delim-col-tests-delimit-columns-format/nil)
(delim-col-tests-delimit-columns-format/padding)
(delim-col-tests-delimit-columns-format/separator)
(delim-col-tests-delimit-columns-separator)
(delim-col-tests-delimit-columns-str-before-after)
(delim-col-tests-delimit-columns-str-separator)
(delim-col-tests-delimit-rectangle): New unit tests.
---
 lisp/delim-col.el            |   2 +-
 test/lisp/delim-col-tests.el | 183 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 184 insertions(+), 1 deletion(-)
 create mode 100644 test/lisp/delim-col-tests.el

diff --git a/lisp/delim-col.el b/lisp/delim-col.el
index a968b32052..77c378ffe6 100644
--- a/lisp/delim-col.el
+++ b/lisp/delim-col.el
@@ -1,4 +1,4 @@
-;;; delim-col.el --- prettify all columns in a region or rectangle
+;;; delim-col.el --- prettify all columns in a region or rectangle  -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 1999-2019 Free Software Foundation, Inc.
 
diff --git a/test/lisp/delim-col-tests.el b/test/lisp/delim-col-tests.el
new file mode 100644
index 0000000000..6a458b1d4a
--- /dev/null
+++ b/test/lisp/delim-col-tests.el
@@ -0,0 +1,183 @@
+;;; delim-col-tests.el --- Tests for delim-col.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2019 Free Software Foundation, Inc.
+
+;; Author: Stefan Kangas <stefankangas@gmail.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'delim-col)
+
+(ert-deftest delim-col-tests-delimit-columns ()
+  (with-temp-buffer
+    (insert "a	b	c\n")
+    (delimit-columns-region (point-min) (point-max))
+    (should (equal (buffer-string) "a, b, c\n")))
+  (with-temp-buffer
+    (insert "a	b	c	d\n"
+            "aaaa	bb	ccc	ddddd\n"
+            "aaa	bbb	cccc	dddd\n"
+            "aa	bb	ccccccc	ddd\n")
+    (delimit-columns-region (point-min) (point-max))
+    (should (equal (buffer-string)
+                   (concat "a,    b,   c,       d    \n"
+                           "aaaa, bb,  ccc,     ddddd\n"
+                           "aaa,  bbb, cccc,    dddd \n"
+                           "aa,   bb,  ccccccc, ddd  \n")))))
+
+(ert-deftest delim-col-tests-delimit-rectangle ()
+  (with-temp-buffer
+    (insert "a	b	c	d\n"
+            "aaaa	bb	ccc	ddddd\n"
+            "aaa	bbb	cccc	dddd\n"
+            "aa	bb	ccccccc	ddd\n")
+    (delimit-columns-rectangle 3 58) ; from first b to last c
+    (should (equal (buffer-string)
+                   (concat "a	b,   c      	d\n"
+                           "aaaa	bb,  ccc    	ddddd\n"
+                           "aaa	bbb, cccc   	dddd\n"
+                           "aa	bb,  ccccccc	ddd\n")))))
+
+(ert-deftest delim-col-tests-delimit-columns-str-separator ()
+  (let ((delimit-columns-str-separator ":"))
+    (with-temp-buffer
+      (insert "a	b\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "a:b\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 16) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	b: c	d\n"
+                             "aa	bb:cc	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-str-before-after ()
+  (let ((delimit-columns-str-before "[ ")
+        (delimit-columns-str-after " ]"))
+    (with-temp-buffer
+      (insert "a	b	c\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "[ a, b, c ]\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aaaa	bb	ccc	ddddd\n"
+              "aaa	bbb	cccc	dddd\n"
+              "aa	bb	ccccccc	ddd\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "[ a,    b,   c,       d     ]\n"
+                             "[ aaaa, bb,  ccc,     ddddd ]\n"
+                             "[ aaa,  bbb, cccc,    dddd  ]\n"
+                             "[ aa,   bb,  ccccccc, ddd   ]\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aaaa	bb	ccc	ddddd\n"
+              "aaa	bbb	cccc	dddd\n"
+              "aa	bb	ccccccc	ddd\n")
+     (delimit-columns-rectangle 3 58)   ; from first b to last c
+     (should (equal (buffer-string)
+                    (concat "a	[ b,   c       ]	d\n"
+                            "aaaa	[ bb,  ccc     ]	ddddd\n"
+                            "aaa	[ bbb, cccc    ]	dddd\n"
+                            "aa	[ bb,  ccccccc ]	ddd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-colummns-before-after ()
+  (let ((delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "<a>, <b>\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17)
+      (should (equal (buffer-string)
+                     (concat "a	<b>,  <c> 	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-separator ()
+  (let ((delimit-columns-separator ","))
+    (with-temp-buffer
+      (insert "a,b,c\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "a, b, c\n")))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/nil ()
+  (let ((delimit-columns-format nil))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "a, b\n"
+                             "aa, bb\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17) ; from first b to last c
+      (buffer-string)
+      (should (equal (buffer-string)
+                     (concat "a	b, c	d\n"
+                             "aa	bb, cc	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/separator ()
+  (let ((delimit-columns-format 'separator)
+        (delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "<a> , <b> \n"
+                             "<aa>, <bb>\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17) ; from first b to last c
+      (buffer-string)
+      (should (equal (buffer-string)
+                     (concat "a	<b> , <c> 	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/padding ()
+  (let ((delimit-columns-format 'padding)
+        (delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (buffer-string)
+      (should (equal (buffer-string) "<a >, <b >\n<aa>, <bb>\n"))
+      )
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17)  ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	<b >, <c >	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(provide 'delim-col-tests)
+;;; delim-col-tests.el ends here
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-07 20:56 [PATCH] Unit tests and lexical-binding for delim-col.el Stefan Kangas
@ 2019-05-07 22:44 ` Basil L. Contovounesios
  2019-05-07 23:03   ` Noam Postavsky
  2019-05-08  7:36   ` Stefan Kangas
  0 siblings, 2 replies; 12+ messages in thread
From: Basil L. Contovounesios @ 2019-05-07 22:44 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: emacs-devel

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

Stefan Kangas <stefan@marxist.se> writes:

> I was looking into some area where I could contribute and ended up writing unit
> tests for `delim-col.el' and adding the lexical-binding header.

Thanks for working on this.

> I wasn't inclined to convert the code in delim-col.el to actually take advantage
> of lexical bindings, but it compiles and runs fine with just naively adding the
> header.
>
> Please let me know what you think or if I've made any obvious
> mistakes.

Only a minor question from me, see below.

> PS. Since this patch is longer than 15 lines, I have initiated the copyright
> assignment process.  It wasn't obvious if this should be done before or after
> mailing this list.

Speaking without authority, I don't think it matters.  There's no harm
in starting discussions early.  In fact, it's usually more beneficial
that way, to potentially save any unnecessary development effort and
give people more time to chime in and give things a whirl.

> +(ert-deftest delim-col-tests-delimit-columns-format/nil ()
> +  (let ((delimit-columns-format nil))
> +    (with-temp-buffer
> +      (insert "a	b\n"
> +              "aa	bb\n")
> +      (delimit-columns-region (point-min) (point-max))
> +      (should (equal (buffer-string)
> +                     (concat "a, b\n"
> +                             "aa, bb\n"))))
> +    (with-temp-buffer
> +      (insert "a	b	c	d\n"
> +              "aa	bb	cc	dd\n")
> +      (delimit-columns-rectangle 3 17) ; from first b to last c
> +      (buffer-string)
> +      (should (equal (buffer-string)
> +                     (concat "a	b, c	d\n"
> +                             "aa	bb, cc	dd\n"))))))

Why is buffer-string called twice in this and the following two tests?

> +(ert-deftest delim-col-tests-delimit-columns-format/separator ()
> +  (let ((delimit-columns-format 'separator)
> +        (delimit-columns-before "<")
> +        (delimit-columns-after ">"))
> +    (with-temp-buffer
> +      (insert "a	b\n"
> +              "aa	bb\n")
> +      (delimit-columns-region (point-min) (point-max))
> +      (should (equal (buffer-string)
> +                     (concat "<a> , <b> \n"
> +                             "<aa>, <bb>\n"))))
> +    (with-temp-buffer
> +      (insert "a	b	c	d\n"
> +              "aa	bb	cc	dd\n")
> +      (delimit-columns-rectangle 3 17) ; from first b to last c
> +      (buffer-string)
> +      (should (equal (buffer-string)
> +                     (concat "a	<b> , <c> 	d\n"
> +                             "aa	<bb>, <cc>	dd\n"))))))
> +
> +(ert-deftest delim-col-tests-delimit-columns-format/padding ()
> +  (let ((delimit-columns-format 'padding)
> +        (delimit-columns-before "<")
> +        (delimit-columns-after ">"))
> +    (with-temp-buffer
> +      (insert "a	b\n"
> +              "aa	bb\n")
> +      (delimit-columns-region (point-min) (point-max))
> +      (buffer-string)
> +      (should (equal (buffer-string) "<a >, <b >\n<aa>, <bb>\n"))
> +      )
> +    (with-temp-buffer
> +      (insert "a	b	c	d\n"
> +              "aa	bb	cc	dd\n")
> +      (delimit-columns-rectangle 3 17)  ; from first b to last c
> +      (should (equal (buffer-string)
> +                     (concat "a	<b >, <c >	d\n"
> +                             "aa	<bb>, <cc>	dd\n"))))))

You said you were disinclined to adapt delim-col.el itself, but how
about bundling the following minor cleanups as well?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: delim-col.diff --]
[-- Type: text/x-diff, Size: 6095 bytes --]

diff --git a/lisp/delim-col.el b/lisp/delim-col.el
index a968b32052..85fc06652d 100644
--- a/lisp/delim-col.el
+++ b/lisp/delim-col.el
@@ -91,9 +91,9 @@
 ;;	aaa	[ <bbb>, <cccc>    ]	dddd
 ;;	aa	[ <bb> , <ccccccc> ]	ddd
 ;;
-;; Note that `delimit-columns-region' operates over all text region
-;; selected, extending the region start to the beginning of line and the
-;; region end to the end of line.  While `delimit-columns-rectangle'
+;; Note that `delimit-columns-region' operates over the entire selected
+;; text region, extending the region start to the beginning of line and
+;; the region end to the end of line.  While `delimit-columns-rectangle'
 ;; operates over the text rectangle selected which rectangle diagonal is
 ;; given by the region start and end.
 ;;
@@ -117,6 +117,7 @@
 
 ;;; Code:
 
+(require 'rect)
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; User Options:
@@ -213,10 +214,11 @@ delimit-columns-start
 The following relation must hold:
    0 <= delimit-columns-start <= delimit-columns-end
 
-The column number start from 0 and it's relative to the beginning of selected
-region.  So if you selected a text region, the first column (column 0) is
-located at beginning of line.  If you selected a text rectangle, the first
-column (column 0) is located at left corner."
+The column number starts at 0 and is relative to the beginning of
+the selected region.  So if you select a text region, the first
+column (column 0) is located at the beginning of line.  If you
+select a text rectangle, the first column (column 0) is located
+at the left corner."
   :type '(integer :tag "Column Start")
   :group 'columns)
 
@@ -228,10 +230,11 @@ delimit-columns-end
 The following relation must hold:
    0 <= delimit-columns-start <= delimit-columns-end
 
-The column number start from 0 and it's relative to the beginning of selected
-region.  So if you selected a text region, the first column (column 0) is
-located at beginning of line.  If you selected a text rectangle, the first
-column (column 0) is located at left corner."
+The column number starts at 0 and is relative to the beginning of
+the selected region.  So if you select a text region, the first
+column (column 0) is located at the beginning of line.  If you
+select a text rectangle, the first column (column 0) is located
+at the left corner."
   :type '(integer :tag "Column End")
   :group 'columns)
 
@@ -247,20 +250,20 @@ delimit-columns-limit
 
 ;;;###autoload
 (defun delimit-columns-customize ()
-  "Customization of `columns' group."
+  "Customize the `columns' group."
   (interactive)
   (customize-group 'columns))
 
 
-(defmacro delimit-columns-str (str)
-  `(if (stringp ,str) ,str ""))
+(defsubst delimit-columns-str (str)
+  (if (stringp str) str ""))
 
 
 ;;;###autoload
 (defun delimit-columns-region (start end)
   "Prettify all columns in a text region.
 
-START and END delimits the text region."
+START and END delimit the text region."
   (interactive "*r")
   (let ((delimit-columns-str-before
 	 (delimit-columns-str delimit-columns-str-before))
@@ -273,8 +276,7 @@ delimit-columns-region
 	(delimit-columns-after
 	 (delimit-columns-str delimit-columns-after))
 	(delimit-columns-start
-	 (if (and (integerp delimit-columns-start)
-		  (>= delimit-columns-start 0))
+         (if (natnump delimit-columns-start)
 	     delimit-columns-start
 	   0))
 	(delimit-columns-end
@@ -309,14 +311,11 @@ delimit-columns-region
 	(set-marker the-end nil)))))
 
 
-(require 'rect)
-
-
 ;;;###autoload
 (defun delimit-columns-rectangle (start end)
   "Prettify all columns in a text rectangle.
 
-START and END delimits the corners of text rectangle."
+START and END delimit the corners of the text rectangle."
   (interactive "*r")
   (let ((delimit-columns-str-before
 	 (delimit-columns-str delimit-columns-str-before))
@@ -329,8 +328,7 @@ delimit-columns-rectangle
 	(delimit-columns-after
 	 (delimit-columns-str delimit-columns-after))
 	(delimit-columns-start
-	 (if (and (integerp delimit-columns-start)
-		  (>= delimit-columns-start 0))
+         (if (natnump delimit-columns-start)
 	     delimit-columns-start
 	   0))
 	(delimit-columns-end
@@ -344,11 +342,11 @@ delimit-columns-rectangle
       ;; get maximum length for each column
       (and delimit-columns-format
 	   (save-excursion
-	     (operate-on-rectangle 'delimit-columns-rectangle-max
+             (operate-on-rectangle #'delimit-columns-rectangle-max
 				   start the-end nil)))
       ;; prettify columns
       (save-excursion
-	(operate-on-rectangle 'delimit-columns-rectangle-line
+        (operate-on-rectangle #'delimit-columns-rectangle-line
 			      start the-end nil))
       ;; nullify markers
       (set-marker delimit-columns-limit nil)
@@ -359,7 +357,7 @@ delimit-columns-rectangle
 ;; Internal Variables and Functions:
 
 
-(defun delimit-columns-rectangle-max (startpos &optional _ignore1 _ignore2)
+(defun delimit-columns-rectangle-max (startpos &optional _begextra _endextra)
   (set-marker delimit-columns-limit (point))
   (goto-char startpos)
   (let ((ncol 1)
@@ -392,7 +390,7 @@ delimit-columns-rectangle-max
       (setq values (cdr values)))))
 
 
-(defun delimit-columns-rectangle-line (startpos &optional _ignore1 _ignore2)
+(defun delimit-columns-rectangle-line (startpos &optional _begextra _endextra)
   (let ((len  (length delimit-columns-max))
 	(ncol 0)
 	origin)
@@ -442,8 +440,7 @@ delimit-columns-rectangle-line
 	    ((eq delimit-columns-format 'padding)
 	     (insert spaces delimit-columns-after delimit-columns-str-after))
 	    (t
-	     (insert delimit-columns-after spaces delimit-columns-str-after))
-	    ))
+             (insert delimit-columns-after spaces delimit-columns-str-after))))
     (goto-char (max (point) delimit-columns-limit))))
 
 
@@ -466,8 +463,7 @@ delimit-columns-format
 	 (insert delimit-columns-after
 		 delimit-columns-str-separator
 		 spaces
-		 delimit-columns-before))
-	))
+                 delimit-columns-before))))
 
 \f
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

[-- Attachment #3: Type: text/plain, Size: 20 bytes --]


Thanks,

-- 
Basil

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-07 22:44 ` Basil L. Contovounesios
@ 2019-05-07 23:03   ` Noam Postavsky
  2019-05-08  7:36   ` Stefan Kangas
  1 sibling, 0 replies; 12+ messages in thread
From: Noam Postavsky @ 2019-05-07 23:03 UTC (permalink / raw)
  To: Basil L. Contovounesios; +Cc: Stefan Kangas, Emacs developers

On Tue, 7 May 2019 at 18:45, Basil L. Contovounesios <contovob@tcd.ie> wrote:

> You said you were disinclined to adapt delim-col.el itself, but how
> about bundling the following minor cleanups as well?

> -(defmacro delimit-columns-str (str)
> -  `(if (stringp ,str) ,str ""))
> +(defsubst delimit-columns-str (str)
> +  (if (stringp str) str ""))

I think you may as well go with defun here, delimit-columns-str isn't
being used in some performance-critical spot, as far as I can tell.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-07 22:44 ` Basil L. Contovounesios
  2019-05-07 23:03   ` Noam Postavsky
@ 2019-05-08  7:36   ` Stefan Kangas
  2019-05-08 12:00     ` Basil L. Contovounesios
  1 sibling, 1 reply; 12+ messages in thread
From: Stefan Kangas @ 2019-05-08  7:36 UTC (permalink / raw)
  To: Basil L. Contovounesios; +Cc: emacs-devel


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

Basil L. Contovounesios <contovob@tcd.ie> writes:
> Only a minor question from me, see below.
> [...]
> Why is buffer-string called twice in this and the following two tests?

Thank you for your input.  These calls were left there by mistake, fixed in
the
attached patch.

> You said you were disinclined to adapt delim-col.el itself, but how
> about bundling the following minor cleanups as well?

Looks good to me.  I've included your cleanups in the attached patch.

Noam Postavsky <npostavs@gmail.com> writes:
> I think you may as well go with defun here, delimit-columns-str isn't
> being used in some performance-critical spot, as far as I can tell.

You are probably correct.  However, I decided to leave Basil's changes as
is,
since I don't know what would conventionally be considered cleaner/better in
this case.

Stefan Kangas

[-- Attachment #1.2: Type: text/html, Size: 1217 bytes --]

[-- Attachment #2: 0001-lisp-delim-col.el-Use-lexical-binding.patch --]
[-- Type: text/x-patch, Size: 14469 bytes --]

From 19db94b6182d492e975f6119cc11e09572a9242d Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 5 May 2019 15:48:57 +0200
Subject: [PATCH] * lisp/delim-col.el: Use lexical-binding

* test/lisp/delim-col-tests.el (delim-col-tests-delimit-colummns-before-after)
(delim-col-tests-delimit-columns)
(delim-col-tests-delimit-columns-format/nil)
(delim-col-tests-delimit-columns-format/padding)
(delim-col-tests-delimit-columns-format/separator)
(delim-col-tests-delimit-columns-separator)
(delim-col-tests-delimit-columns-str-before-after)
(delim-col-tests-delimit-columns-str-separator)
(delim-col-tests-delimit-rectangle): New unit tests.
---
 lisp/delim-col.el            |  60 +++++++-------
 test/lisp/delim-col-tests.el | 181 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+), 32 deletions(-)
 create mode 100644 test/lisp/delim-col-tests.el

diff --git a/lisp/delim-col.el b/lisp/delim-col.el
index a968b32052..b696ac6122 100644
--- a/lisp/delim-col.el
+++ b/lisp/delim-col.el
@@ -1,4 +1,4 @@
-;;; delim-col.el --- prettify all columns in a region or rectangle
+;;; delim-col.el --- prettify all columns in a region or rectangle  -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 1999-2019 Free Software Foundation, Inc.
 
@@ -91,9 +91,9 @@
 ;;	aaa	[ <bbb>, <cccc>    ]	dddd
 ;;	aa	[ <bb> , <ccccccc> ]	ddd
 ;;
-;; Note that `delimit-columns-region' operates over all text region
-;; selected, extending the region start to the beginning of line and the
-;; region end to the end of line.  While `delimit-columns-rectangle'
+;; Note that `delimit-columns-region' operates over the entire selected
+;; text region, extending the region start to the beginning of line and
+;; the region end to the end of line.  While `delimit-columns-rectangle'
 ;; operates over the text rectangle selected which rectangle diagonal is
 ;; given by the region start and end.
 ;;
@@ -117,6 +117,7 @@
 
 ;;; Code:
 
+(require 'rect)
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; User Options:
@@ -213,10 +214,11 @@ delimit-columns-start
 The following relation must hold:
    0 <= delimit-columns-start <= delimit-columns-end
 
-The column number start from 0 and it's relative to the beginning of selected
-region.  So if you selected a text region, the first column (column 0) is
-located at beginning of line.  If you selected a text rectangle, the first
-column (column 0) is located at left corner."
+The column number starts at 0 and is relative to the beginning of
+the selected region.  So if you select a text region, the first
+column (column 0) is located at the beginning of line.  If you
+select a text rectangle, the first column (column 0) is located
+at the left corner."
   :type '(integer :tag "Column Start")
   :group 'columns)
 
@@ -228,10 +230,11 @@ delimit-columns-end
 The following relation must hold:
    0 <= delimit-columns-start <= delimit-columns-end
 
-The column number start from 0 and it's relative to the beginning of selected
-region.  So if you selected a text region, the first column (column 0) is
-located at beginning of line.  If you selected a text rectangle, the first
-column (column 0) is located at left corner."
+The column number starts at 0 and is relative to the beginning of
+the selected region.  So if you select a text region, the first
+column (column 0) is located at the beginning of line.  If you
+select a text rectangle, the first column (column 0) is located
+at the left corner."
   :type '(integer :tag "Column End")
   :group 'columns)
 
@@ -247,20 +250,20 @@ delimit-columns-limit
 
 ;;;###autoload
 (defun delimit-columns-customize ()
-  "Customization of `columns' group."
+  "Customize the `columns' group."
   (interactive)
   (customize-group 'columns))
 
 
-(defmacro delimit-columns-str (str)
-  `(if (stringp ,str) ,str ""))
+(defsubst delimit-columns-str (str)
+  (if (stringp str) str ""))
 
 
 ;;;###autoload
 (defun delimit-columns-region (start end)
   "Prettify all columns in a text region.
 
-START and END delimits the text region."
+START and END delimit the text region."
   (interactive "*r")
   (let ((delimit-columns-str-before
 	 (delimit-columns-str delimit-columns-str-before))
@@ -273,8 +276,7 @@ delimit-columns-region
 	(delimit-columns-after
 	 (delimit-columns-str delimit-columns-after))
 	(delimit-columns-start
-	 (if (and (integerp delimit-columns-start)
-		  (>= delimit-columns-start 0))
+         (if (natnump delimit-columns-start)
 	     delimit-columns-start
 	   0))
 	(delimit-columns-end
@@ -309,14 +311,11 @@ delimit-columns-region
 	(set-marker the-end nil)))))
 
 
-(require 'rect)
-
-
 ;;;###autoload
 (defun delimit-columns-rectangle (start end)
   "Prettify all columns in a text rectangle.
 
-START and END delimits the corners of text rectangle."
+START and END delimit the corners of the text rectangle."
   (interactive "*r")
   (let ((delimit-columns-str-before
 	 (delimit-columns-str delimit-columns-str-before))
@@ -329,8 +328,7 @@ delimit-columns-rectangle
 	(delimit-columns-after
 	 (delimit-columns-str delimit-columns-after))
 	(delimit-columns-start
-	 (if (and (integerp delimit-columns-start)
-		  (>= delimit-columns-start 0))
+         (if (natnump delimit-columns-start)
 	     delimit-columns-start
 	   0))
 	(delimit-columns-end
@@ -344,11 +342,11 @@ delimit-columns-rectangle
       ;; get maximum length for each column
       (and delimit-columns-format
 	   (save-excursion
-	     (operate-on-rectangle 'delimit-columns-rectangle-max
+             (operate-on-rectangle #'delimit-columns-rectangle-max
 				   start the-end nil)))
       ;; prettify columns
       (save-excursion
-	(operate-on-rectangle 'delimit-columns-rectangle-line
+        (operate-on-rectangle #'delimit-columns-rectangle-line
 			      start the-end nil))
       ;; nullify markers
       (set-marker delimit-columns-limit nil)
@@ -359,7 +357,7 @@ delimit-columns-rectangle
 ;; Internal Variables and Functions:
 
 
-(defun delimit-columns-rectangle-max (startpos &optional _ignore1 _ignore2)
+(defun delimit-columns-rectangle-max (startpos &optional _begextra _endextra)
   (set-marker delimit-columns-limit (point))
   (goto-char startpos)
   (let ((ncol 1)
@@ -392,7 +390,7 @@ delimit-columns-rectangle-max
       (setq values (cdr values)))))
 
 
-(defun delimit-columns-rectangle-line (startpos &optional _ignore1 _ignore2)
+(defun delimit-columns-rectangle-line (startpos &optional _begextra _endextra)
   (let ((len  (length delimit-columns-max))
 	(ncol 0)
 	origin)
@@ -442,8 +440,7 @@ delimit-columns-rectangle-line
 	    ((eq delimit-columns-format 'padding)
 	     (insert spaces delimit-columns-after delimit-columns-str-after))
 	    (t
-	     (insert delimit-columns-after spaces delimit-columns-str-after))
-	    ))
+             (insert delimit-columns-after spaces delimit-columns-str-after))))
     (goto-char (max (point) delimit-columns-limit))))
 
 
@@ -466,8 +463,7 @@ delimit-columns-format
 	 (insert delimit-columns-after
 		 delimit-columns-str-separator
 		 spaces
-		 delimit-columns-before))
-	))
+                 delimit-columns-before))))
 
 \f
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
diff --git a/test/lisp/delim-col-tests.el b/test/lisp/delim-col-tests.el
new file mode 100644
index 0000000000..f2a0377b07
--- /dev/null
+++ b/test/lisp/delim-col-tests.el
@@ -0,0 +1,181 @@
+;;; delim-col-tests.el --- Tests for delim-col.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2019 Free Software Foundation, Inc.
+
+;; Author: Stefan Kangas <stefankangas@gmail.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'delim-col)
+
+(ert-deftest delim-col-tests-delimit-columns ()
+  (with-temp-buffer
+    (insert "a	b	c\n")
+    (delimit-columns-region (point-min) (point-max))
+    (should (equal (buffer-string) "a, b, c\n")))
+  (with-temp-buffer
+    (insert "a	b	c	d\n"
+            "aaaa	bb	ccc	ddddd\n"
+            "aaa	bbb	cccc	dddd\n"
+            "aa	bb	ccccccc	ddd\n")
+    (delimit-columns-region (point-min) (point-max))
+    (should (equal (buffer-string)
+                   (concat "a,    b,   c,       d    \n"
+                           "aaaa, bb,  ccc,     ddddd\n"
+                           "aaa,  bbb, cccc,    dddd \n"
+                           "aa,   bb,  ccccccc, ddd  \n")))))
+
+(ert-deftest delim-col-tests-delimit-rectangle ()
+  (with-temp-buffer
+    (insert "a	b	c	d\n"
+            "aaaa	bb	ccc	ddddd\n"
+            "aaa	bbb	cccc	dddd\n"
+            "aa	bb	ccccccc	ddd\n")
+    (delimit-columns-rectangle 3 58) ; from first b to last c
+    (should (equal (buffer-string)
+                   (concat "a	b,   c      	d\n"
+                           "aaaa	bb,  ccc    	ddddd\n"
+                           "aaa	bbb, cccc   	dddd\n"
+                           "aa	bb,  ccccccc	ddd\n")))))
+
+(ert-deftest delim-col-tests-delimit-columns-str-separator ()
+  (let ((delimit-columns-str-separator ":"))
+    (with-temp-buffer
+      (insert "a	b\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "a:b\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 16) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	b: c	d\n"
+                             "aa	bb:cc	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-str-before-after ()
+  (let ((delimit-columns-str-before "[ ")
+        (delimit-columns-str-after " ]"))
+    (with-temp-buffer
+      (insert "a	b	c\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "[ a, b, c ]\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aaaa	bb	ccc	ddddd\n"
+              "aaa	bbb	cccc	dddd\n"
+              "aa	bb	ccccccc	ddd\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "[ a,    b,   c,       d     ]\n"
+                             "[ aaaa, bb,  ccc,     ddddd ]\n"
+                             "[ aaa,  bbb, cccc,    dddd  ]\n"
+                             "[ aa,   bb,  ccccccc, ddd   ]\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aaaa	bb	ccc	ddddd\n"
+              "aaa	bbb	cccc	dddd\n"
+              "aa	bb	ccccccc	ddd\n")
+     (delimit-columns-rectangle 3 58)   ; from first b to last c
+     (should (equal (buffer-string)
+                    (concat "a	[ b,   c       ]	d\n"
+                            "aaaa	[ bb,  ccc     ]	ddddd\n"
+                            "aaa	[ bbb, cccc    ]	dddd\n"
+                            "aa	[ bb,  ccccccc ]	ddd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-colummns-before-after ()
+  (let ((delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "<a>, <b>\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17)
+      (should (equal (buffer-string)
+                     (concat "a	<b>,  <c> 	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-separator ()
+  (let ((delimit-columns-separator ","))
+    (with-temp-buffer
+      (insert "a,b,c\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "a, b, c\n")))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/nil ()
+  (let ((delimit-columns-format nil))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "a, b\n"
+                             "aa, bb\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	b, c	d\n"
+                             "aa	bb, cc	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/separator ()
+  (let ((delimit-columns-format 'separator)
+        (delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "<a> , <b> \n"
+                             "<aa>, <bb>\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	<b> , <c> 	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/padding ()
+  (let ((delimit-columns-format 'padding)
+        (delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "<a >, <b >\n"
+                             "<aa>, <bb>\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17)  ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	<b >, <c >	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(provide 'delim-col-tests)
+;;; delim-col-tests.el ends here
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-08  7:36   ` Stefan Kangas
@ 2019-05-08 12:00     ` Basil L. Contovounesios
  2019-05-08 17:20       ` Stefan Kangas
  0 siblings, 1 reply; 12+ messages in thread
From: Basil L. Contovounesios @ 2019-05-08 12:00 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: emacs-devel

Stefan Kangas <stefan@marxist.se> writes:

> Noam Postavsky <npostavs@gmail.com> writes:
>> I think you may as well go with defun here, delimit-columns-str isn't
>> being used in some performance-critical spot, as far as I can tell.
>
> You are probably correct.  However, I decided to leave Basil's changes as is,
> since I don't know what would conventionally be considered cleaner/better in
> this case.

It is always better to use defun until proven otherwise, see (info
"(elisp) Inline Functions").  The only reason I used defsubst was to be
more conservative, but if no-one minds then defun is indeed preferable.

Thanks,

-- 
Basil



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-08 12:00     ` Basil L. Contovounesios
@ 2019-05-08 17:20       ` Stefan Kangas
  2019-05-08 20:24         ` Stefan Kangas
  0 siblings, 1 reply; 12+ messages in thread
From: Stefan Kangas @ 2019-05-08 17:20 UTC (permalink / raw)
  To: Basil L. Contovounesios; +Cc: emacs-devel


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

Basil L. Contovounesios <contovob@tcd.ie> writes:
> It is always better to use defun until proven otherwise, see (info
> "(elisp) Inline Functions").  The only reason I used defsubst was to be
> more conservative, but if no-one minds then defun is indeed preferable.

I was similarly trying to be conservative, maybe overly so.  I've attached
an
updated patch that also gets rid of the defsubst.  Thanks again for your
patience.

Stefan Kangas

[-- Attachment #1.2: Type: text/html, Size: 642 bytes --]

[-- Attachment #2: 0001-lisp-delim-col.el-Use-lexical-binding-3.patch --]
[-- Type: text/x-patch, Size: 14466 bytes --]

From 34ce4788492931055e94cb8a4c17afa290fbcf58 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 5 May 2019 15:48:57 +0200
Subject: [PATCH] * lisp/delim-col.el: Use lexical-binding

* test/lisp/delim-col-tests.el (delim-col-tests-delimit-colummns-before-after)
(delim-col-tests-delimit-columns)
(delim-col-tests-delimit-columns-format/nil)
(delim-col-tests-delimit-columns-format/padding)
(delim-col-tests-delimit-columns-format/separator)
(delim-col-tests-delimit-columns-separator)
(delim-col-tests-delimit-columns-str-before-after)
(delim-col-tests-delimit-columns-str-separator)
(delim-col-tests-delimit-rectangle): New unit tests.
---
 lisp/delim-col.el            |  60 +++++++-------
 test/lisp/delim-col-tests.el | 181 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+), 32 deletions(-)
 create mode 100644 test/lisp/delim-col-tests.el

diff --git a/lisp/delim-col.el b/lisp/delim-col.el
index a968b32052..3c48972b74 100644
--- a/lisp/delim-col.el
+++ b/lisp/delim-col.el
@@ -1,4 +1,4 @@
-;;; delim-col.el --- prettify all columns in a region or rectangle
+;;; delim-col.el --- prettify all columns in a region or rectangle  -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 1999-2019 Free Software Foundation, Inc.
 
@@ -91,9 +91,9 @@
 ;;	aaa	[ <bbb>, <cccc>    ]	dddd
 ;;	aa	[ <bb> , <ccccccc> ]	ddd
 ;;
-;; Note that `delimit-columns-region' operates over all text region
-;; selected, extending the region start to the beginning of line and the
-;; region end to the end of line.  While `delimit-columns-rectangle'
+;; Note that `delimit-columns-region' operates over the entire selected
+;; text region, extending the region start to the beginning of line and
+;; the region end to the end of line.  While `delimit-columns-rectangle'
 ;; operates over the text rectangle selected which rectangle diagonal is
 ;; given by the region start and end.
 ;;
@@ -117,6 +117,7 @@
 
 ;;; Code:
 
+(require 'rect)
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; User Options:
@@ -213,10 +214,11 @@ delimit-columns-start
 The following relation must hold:
    0 <= delimit-columns-start <= delimit-columns-end
 
-The column number start from 0 and it's relative to the beginning of selected
-region.  So if you selected a text region, the first column (column 0) is
-located at beginning of line.  If you selected a text rectangle, the first
-column (column 0) is located at left corner."
+The column number starts at 0 and is relative to the beginning of
+the selected region.  So if you select a text region, the first
+column (column 0) is located at the beginning of line.  If you
+select a text rectangle, the first column (column 0) is located
+at the left corner."
   :type '(integer :tag "Column Start")
   :group 'columns)
 
@@ -228,10 +230,11 @@ delimit-columns-end
 The following relation must hold:
    0 <= delimit-columns-start <= delimit-columns-end
 
-The column number start from 0 and it's relative to the beginning of selected
-region.  So if you selected a text region, the first column (column 0) is
-located at beginning of line.  If you selected a text rectangle, the first
-column (column 0) is located at left corner."
+The column number starts at 0 and is relative to the beginning of
+the selected region.  So if you select a text region, the first
+column (column 0) is located at the beginning of line.  If you
+select a text rectangle, the first column (column 0) is located
+at the left corner."
   :type '(integer :tag "Column End")
   :group 'columns)
 
@@ -247,20 +250,20 @@ delimit-columns-limit
 
 ;;;###autoload
 (defun delimit-columns-customize ()
-  "Customization of `columns' group."
+  "Customize the `columns' group."
   (interactive)
   (customize-group 'columns))
 
 
-(defmacro delimit-columns-str (str)
-  `(if (stringp ,str) ,str ""))
+(defun delimit-columns-str (str)
+  (if (stringp str) str ""))
 
 
 ;;;###autoload
 (defun delimit-columns-region (start end)
   "Prettify all columns in a text region.
 
-START and END delimits the text region."
+START and END delimit the text region."
   (interactive "*r")
   (let ((delimit-columns-str-before
 	 (delimit-columns-str delimit-columns-str-before))
@@ -273,8 +276,7 @@ delimit-columns-region
 	(delimit-columns-after
 	 (delimit-columns-str delimit-columns-after))
 	(delimit-columns-start
-	 (if (and (integerp delimit-columns-start)
-		  (>= delimit-columns-start 0))
+         (if (natnump delimit-columns-start)
 	     delimit-columns-start
 	   0))
 	(delimit-columns-end
@@ -309,14 +311,11 @@ delimit-columns-region
 	(set-marker the-end nil)))))
 
 
-(require 'rect)
-
-
 ;;;###autoload
 (defun delimit-columns-rectangle (start end)
   "Prettify all columns in a text rectangle.
 
-START and END delimits the corners of text rectangle."
+START and END delimit the corners of the text rectangle."
   (interactive "*r")
   (let ((delimit-columns-str-before
 	 (delimit-columns-str delimit-columns-str-before))
@@ -329,8 +328,7 @@ delimit-columns-rectangle
 	(delimit-columns-after
 	 (delimit-columns-str delimit-columns-after))
 	(delimit-columns-start
-	 (if (and (integerp delimit-columns-start)
-		  (>= delimit-columns-start 0))
+         (if (natnump delimit-columns-start)
 	     delimit-columns-start
 	   0))
 	(delimit-columns-end
@@ -344,11 +342,11 @@ delimit-columns-rectangle
       ;; get maximum length for each column
       (and delimit-columns-format
 	   (save-excursion
-	     (operate-on-rectangle 'delimit-columns-rectangle-max
+             (operate-on-rectangle #'delimit-columns-rectangle-max
 				   start the-end nil)))
       ;; prettify columns
       (save-excursion
-	(operate-on-rectangle 'delimit-columns-rectangle-line
+        (operate-on-rectangle #'delimit-columns-rectangle-line
 			      start the-end nil))
       ;; nullify markers
       (set-marker delimit-columns-limit nil)
@@ -359,7 +357,7 @@ delimit-columns-rectangle
 ;; Internal Variables and Functions:
 
 
-(defun delimit-columns-rectangle-max (startpos &optional _ignore1 _ignore2)
+(defun delimit-columns-rectangle-max (startpos &optional _begextra _endextra)
   (set-marker delimit-columns-limit (point))
   (goto-char startpos)
   (let ((ncol 1)
@@ -392,7 +390,7 @@ delimit-columns-rectangle-max
       (setq values (cdr values)))))
 
 
-(defun delimit-columns-rectangle-line (startpos &optional _ignore1 _ignore2)
+(defun delimit-columns-rectangle-line (startpos &optional _begextra _endextra)
   (let ((len  (length delimit-columns-max))
 	(ncol 0)
 	origin)
@@ -442,8 +440,7 @@ delimit-columns-rectangle-line
 	    ((eq delimit-columns-format 'padding)
 	     (insert spaces delimit-columns-after delimit-columns-str-after))
 	    (t
-	     (insert delimit-columns-after spaces delimit-columns-str-after))
-	    ))
+             (insert delimit-columns-after spaces delimit-columns-str-after))))
     (goto-char (max (point) delimit-columns-limit))))
 
 
@@ -466,8 +463,7 @@ delimit-columns-format
 	 (insert delimit-columns-after
 		 delimit-columns-str-separator
 		 spaces
-		 delimit-columns-before))
-	))
+                 delimit-columns-before))))
 
 \f
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
diff --git a/test/lisp/delim-col-tests.el b/test/lisp/delim-col-tests.el
new file mode 100644
index 0000000000..f2a0377b07
--- /dev/null
+++ b/test/lisp/delim-col-tests.el
@@ -0,0 +1,181 @@
+;;; delim-col-tests.el --- Tests for delim-col.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2019 Free Software Foundation, Inc.
+
+;; Author: Stefan Kangas <stefankangas@gmail.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'delim-col)
+
+(ert-deftest delim-col-tests-delimit-columns ()
+  (with-temp-buffer
+    (insert "a	b	c\n")
+    (delimit-columns-region (point-min) (point-max))
+    (should (equal (buffer-string) "a, b, c\n")))
+  (with-temp-buffer
+    (insert "a	b	c	d\n"
+            "aaaa	bb	ccc	ddddd\n"
+            "aaa	bbb	cccc	dddd\n"
+            "aa	bb	ccccccc	ddd\n")
+    (delimit-columns-region (point-min) (point-max))
+    (should (equal (buffer-string)
+                   (concat "a,    b,   c,       d    \n"
+                           "aaaa, bb,  ccc,     ddddd\n"
+                           "aaa,  bbb, cccc,    dddd \n"
+                           "aa,   bb,  ccccccc, ddd  \n")))))
+
+(ert-deftest delim-col-tests-delimit-rectangle ()
+  (with-temp-buffer
+    (insert "a	b	c	d\n"
+            "aaaa	bb	ccc	ddddd\n"
+            "aaa	bbb	cccc	dddd\n"
+            "aa	bb	ccccccc	ddd\n")
+    (delimit-columns-rectangle 3 58) ; from first b to last c
+    (should (equal (buffer-string)
+                   (concat "a	b,   c      	d\n"
+                           "aaaa	bb,  ccc    	ddddd\n"
+                           "aaa	bbb, cccc   	dddd\n"
+                           "aa	bb,  ccccccc	ddd\n")))))
+
+(ert-deftest delim-col-tests-delimit-columns-str-separator ()
+  (let ((delimit-columns-str-separator ":"))
+    (with-temp-buffer
+      (insert "a	b\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "a:b\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 16) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	b: c	d\n"
+                             "aa	bb:cc	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-str-before-after ()
+  (let ((delimit-columns-str-before "[ ")
+        (delimit-columns-str-after " ]"))
+    (with-temp-buffer
+      (insert "a	b	c\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "[ a, b, c ]\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aaaa	bb	ccc	ddddd\n"
+              "aaa	bbb	cccc	dddd\n"
+              "aa	bb	ccccccc	ddd\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "[ a,    b,   c,       d     ]\n"
+                             "[ aaaa, bb,  ccc,     ddddd ]\n"
+                             "[ aaa,  bbb, cccc,    dddd  ]\n"
+                             "[ aa,   bb,  ccccccc, ddd   ]\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aaaa	bb	ccc	ddddd\n"
+              "aaa	bbb	cccc	dddd\n"
+              "aa	bb	ccccccc	ddd\n")
+     (delimit-columns-rectangle 3 58)   ; from first b to last c
+     (should (equal (buffer-string)
+                    (concat "a	[ b,   c       ]	d\n"
+                            "aaaa	[ bb,  ccc     ]	ddddd\n"
+                            "aaa	[ bbb, cccc    ]	dddd\n"
+                            "aa	[ bb,  ccccccc ]	ddd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-colummns-before-after ()
+  (let ((delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "<a>, <b>\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17)
+      (should (equal (buffer-string)
+                     (concat "a	<b>,  <c> 	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-separator ()
+  (let ((delimit-columns-separator ","))
+    (with-temp-buffer
+      (insert "a,b,c\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "a, b, c\n")))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/nil ()
+  (let ((delimit-columns-format nil))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "a, b\n"
+                             "aa, bb\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	b, c	d\n"
+                             "aa	bb, cc	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/separator ()
+  (let ((delimit-columns-format 'separator)
+        (delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "<a> , <b> \n"
+                             "<aa>, <bb>\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	<b> , <c> 	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/padding ()
+  (let ((delimit-columns-format 'padding)
+        (delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "<a >, <b >\n"
+                             "<aa>, <bb>\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17)  ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	<b >, <c >	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(provide 'delim-col-tests)
+;;; delim-col-tests.el ends here
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-08 17:20       ` Stefan Kangas
@ 2019-05-08 20:24         ` Stefan Kangas
  2019-05-09 13:39           ` Basil L. Contovounesios
  0 siblings, 1 reply; 12+ messages in thread
From: Stefan Kangas @ 2019-05-08 20:24 UTC (permalink / raw)
  To: emacs-devel


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

I just got back the confirmation for the copyright assignment.  If there
are no
more outstanding issues, I think this is ready to be merged.

I've attached the patch (again) with a better commit message and proper
thanks
given to Basil.

Thanks,
Stefan Kangas

[-- Attachment #1.2: Type: text/html, Size: 482 bytes --]

[-- Attachment #2: 0001-Use-lexical-binding-in-delim-col.el-and-add-tests-4.patch --]
[-- Type: text/x-patch, Size: 14578 bytes --]

From c2080636251d67205aef1a144c83408083638547 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 5 May 2019 15:48:57 +0200
Subject: [PATCH] Use lexical-binding in delim-col.el, and add tests

Thanks to Basil L. Contovounesios for additional cleanups.

* lisp/delim-col.el: Use lexical-binding

* test/lisp/delim-col-tests.el (delim-col-tests-delimit-colummns-before-after)
(delim-col-tests-delimit-columns)
(delim-col-tests-delimit-columns-format/nil)
(delim-col-tests-delimit-columns-format/padding)
(delim-col-tests-delimit-columns-format/separator)
(delim-col-tests-delimit-columns-separator)
(delim-col-tests-delimit-columns-str-before-after)
(delim-col-tests-delimit-columns-str-separator)
(delim-col-tests-delimit-rectangle): New unit tests.
---
 lisp/delim-col.el            |  60 +++++++-------
 test/lisp/delim-col-tests.el | 181 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+), 32 deletions(-)
 create mode 100644 test/lisp/delim-col-tests.el

diff --git a/lisp/delim-col.el b/lisp/delim-col.el
index a968b32052..3c48972b74 100644
--- a/lisp/delim-col.el
+++ b/lisp/delim-col.el
@@ -1,4 +1,4 @@
-;;; delim-col.el --- prettify all columns in a region or rectangle
+;;; delim-col.el --- prettify all columns in a region or rectangle  -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 1999-2019 Free Software Foundation, Inc.
 
@@ -91,9 +91,9 @@
 ;;	aaa	[ <bbb>, <cccc>    ]	dddd
 ;;	aa	[ <bb> , <ccccccc> ]	ddd
 ;;
-;; Note that `delimit-columns-region' operates over all text region
-;; selected, extending the region start to the beginning of line and the
-;; region end to the end of line.  While `delimit-columns-rectangle'
+;; Note that `delimit-columns-region' operates over the entire selected
+;; text region, extending the region start to the beginning of line and
+;; the region end to the end of line.  While `delimit-columns-rectangle'
 ;; operates over the text rectangle selected which rectangle diagonal is
 ;; given by the region start and end.
 ;;
@@ -117,6 +117,7 @@
 
 ;;; Code:
 
+(require 'rect)
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; User Options:
@@ -213,10 +214,11 @@ delimit-columns-start
 The following relation must hold:
    0 <= delimit-columns-start <= delimit-columns-end
 
-The column number start from 0 and it's relative to the beginning of selected
-region.  So if you selected a text region, the first column (column 0) is
-located at beginning of line.  If you selected a text rectangle, the first
-column (column 0) is located at left corner."
+The column number starts at 0 and is relative to the beginning of
+the selected region.  So if you select a text region, the first
+column (column 0) is located at the beginning of line.  If you
+select a text rectangle, the first column (column 0) is located
+at the left corner."
   :type '(integer :tag "Column Start")
   :group 'columns)
 
@@ -228,10 +230,11 @@ delimit-columns-end
 The following relation must hold:
    0 <= delimit-columns-start <= delimit-columns-end
 
-The column number start from 0 and it's relative to the beginning of selected
-region.  So if you selected a text region, the first column (column 0) is
-located at beginning of line.  If you selected a text rectangle, the first
-column (column 0) is located at left corner."
+The column number starts at 0 and is relative to the beginning of
+the selected region.  So if you select a text region, the first
+column (column 0) is located at the beginning of line.  If you
+select a text rectangle, the first column (column 0) is located
+at the left corner."
   :type '(integer :tag "Column End")
   :group 'columns)
 
@@ -247,20 +250,20 @@ delimit-columns-limit
 
 ;;;###autoload
 (defun delimit-columns-customize ()
-  "Customization of `columns' group."
+  "Customize the `columns' group."
   (interactive)
   (customize-group 'columns))
 
 
-(defmacro delimit-columns-str (str)
-  `(if (stringp ,str) ,str ""))
+(defun delimit-columns-str (str)
+  (if (stringp str) str ""))
 
 
 ;;;###autoload
 (defun delimit-columns-region (start end)
   "Prettify all columns in a text region.
 
-START and END delimits the text region."
+START and END delimit the text region."
   (interactive "*r")
   (let ((delimit-columns-str-before
 	 (delimit-columns-str delimit-columns-str-before))
@@ -273,8 +276,7 @@ delimit-columns-region
 	(delimit-columns-after
 	 (delimit-columns-str delimit-columns-after))
 	(delimit-columns-start
-	 (if (and (integerp delimit-columns-start)
-		  (>= delimit-columns-start 0))
+         (if (natnump delimit-columns-start)
 	     delimit-columns-start
 	   0))
 	(delimit-columns-end
@@ -309,14 +311,11 @@ delimit-columns-region
 	(set-marker the-end nil)))))
 
 
-(require 'rect)
-
-
 ;;;###autoload
 (defun delimit-columns-rectangle (start end)
   "Prettify all columns in a text rectangle.
 
-START and END delimits the corners of text rectangle."
+START and END delimit the corners of the text rectangle."
   (interactive "*r")
   (let ((delimit-columns-str-before
 	 (delimit-columns-str delimit-columns-str-before))
@@ -329,8 +328,7 @@ delimit-columns-rectangle
 	(delimit-columns-after
 	 (delimit-columns-str delimit-columns-after))
 	(delimit-columns-start
-	 (if (and (integerp delimit-columns-start)
-		  (>= delimit-columns-start 0))
+         (if (natnump delimit-columns-start)
 	     delimit-columns-start
 	   0))
 	(delimit-columns-end
@@ -344,11 +342,11 @@ delimit-columns-rectangle
       ;; get maximum length for each column
       (and delimit-columns-format
 	   (save-excursion
-	     (operate-on-rectangle 'delimit-columns-rectangle-max
+             (operate-on-rectangle #'delimit-columns-rectangle-max
 				   start the-end nil)))
       ;; prettify columns
       (save-excursion
-	(operate-on-rectangle 'delimit-columns-rectangle-line
+        (operate-on-rectangle #'delimit-columns-rectangle-line
 			      start the-end nil))
       ;; nullify markers
       (set-marker delimit-columns-limit nil)
@@ -359,7 +357,7 @@ delimit-columns-rectangle
 ;; Internal Variables and Functions:
 
 
-(defun delimit-columns-rectangle-max (startpos &optional _ignore1 _ignore2)
+(defun delimit-columns-rectangle-max (startpos &optional _begextra _endextra)
   (set-marker delimit-columns-limit (point))
   (goto-char startpos)
   (let ((ncol 1)
@@ -392,7 +390,7 @@ delimit-columns-rectangle-max
       (setq values (cdr values)))))
 
 
-(defun delimit-columns-rectangle-line (startpos &optional _ignore1 _ignore2)
+(defun delimit-columns-rectangle-line (startpos &optional _begextra _endextra)
   (let ((len  (length delimit-columns-max))
 	(ncol 0)
 	origin)
@@ -442,8 +440,7 @@ delimit-columns-rectangle-line
 	    ((eq delimit-columns-format 'padding)
 	     (insert spaces delimit-columns-after delimit-columns-str-after))
 	    (t
-	     (insert delimit-columns-after spaces delimit-columns-str-after))
-	    ))
+             (insert delimit-columns-after spaces delimit-columns-str-after))))
     (goto-char (max (point) delimit-columns-limit))))
 
 
@@ -466,8 +463,7 @@ delimit-columns-format
 	 (insert delimit-columns-after
 		 delimit-columns-str-separator
 		 spaces
-		 delimit-columns-before))
-	))
+                 delimit-columns-before))))
 
 \f
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
diff --git a/test/lisp/delim-col-tests.el b/test/lisp/delim-col-tests.el
new file mode 100644
index 0000000000..f2a0377b07
--- /dev/null
+++ b/test/lisp/delim-col-tests.el
@@ -0,0 +1,181 @@
+;;; delim-col-tests.el --- Tests for delim-col.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2019 Free Software Foundation, Inc.
+
+;; Author: Stefan Kangas <stefankangas@gmail.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'delim-col)
+
+(ert-deftest delim-col-tests-delimit-columns ()
+  (with-temp-buffer
+    (insert "a	b	c\n")
+    (delimit-columns-region (point-min) (point-max))
+    (should (equal (buffer-string) "a, b, c\n")))
+  (with-temp-buffer
+    (insert "a	b	c	d\n"
+            "aaaa	bb	ccc	ddddd\n"
+            "aaa	bbb	cccc	dddd\n"
+            "aa	bb	ccccccc	ddd\n")
+    (delimit-columns-region (point-min) (point-max))
+    (should (equal (buffer-string)
+                   (concat "a,    b,   c,       d    \n"
+                           "aaaa, bb,  ccc,     ddddd\n"
+                           "aaa,  bbb, cccc,    dddd \n"
+                           "aa,   bb,  ccccccc, ddd  \n")))))
+
+(ert-deftest delim-col-tests-delimit-rectangle ()
+  (with-temp-buffer
+    (insert "a	b	c	d\n"
+            "aaaa	bb	ccc	ddddd\n"
+            "aaa	bbb	cccc	dddd\n"
+            "aa	bb	ccccccc	ddd\n")
+    (delimit-columns-rectangle 3 58) ; from first b to last c
+    (should (equal (buffer-string)
+                   (concat "a	b,   c      	d\n"
+                           "aaaa	bb,  ccc    	ddddd\n"
+                           "aaa	bbb, cccc   	dddd\n"
+                           "aa	bb,  ccccccc	ddd\n")))))
+
+(ert-deftest delim-col-tests-delimit-columns-str-separator ()
+  (let ((delimit-columns-str-separator ":"))
+    (with-temp-buffer
+      (insert "a	b\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "a:b\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 16) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	b: c	d\n"
+                             "aa	bb:cc	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-str-before-after ()
+  (let ((delimit-columns-str-before "[ ")
+        (delimit-columns-str-after " ]"))
+    (with-temp-buffer
+      (insert "a	b	c\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "[ a, b, c ]\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aaaa	bb	ccc	ddddd\n"
+              "aaa	bbb	cccc	dddd\n"
+              "aa	bb	ccccccc	ddd\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "[ a,    b,   c,       d     ]\n"
+                             "[ aaaa, bb,  ccc,     ddddd ]\n"
+                             "[ aaa,  bbb, cccc,    dddd  ]\n"
+                             "[ aa,   bb,  ccccccc, ddd   ]\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aaaa	bb	ccc	ddddd\n"
+              "aaa	bbb	cccc	dddd\n"
+              "aa	bb	ccccccc	ddd\n")
+     (delimit-columns-rectangle 3 58)   ; from first b to last c
+     (should (equal (buffer-string)
+                    (concat "a	[ b,   c       ]	d\n"
+                            "aaaa	[ bb,  ccc     ]	ddddd\n"
+                            "aaa	[ bbb, cccc    ]	dddd\n"
+                            "aa	[ bb,  ccccccc ]	ddd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-colummns-before-after ()
+  (let ((delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "<a>, <b>\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17)
+      (should (equal (buffer-string)
+                     (concat "a	<b>,  <c> 	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-separator ()
+  (let ((delimit-columns-separator ","))
+    (with-temp-buffer
+      (insert "a,b,c\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "a, b, c\n")))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/nil ()
+  (let ((delimit-columns-format nil))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "a, b\n"
+                             "aa, bb\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	b, c	d\n"
+                             "aa	bb, cc	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/separator ()
+  (let ((delimit-columns-format 'separator)
+        (delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "<a> , <b> \n"
+                             "<aa>, <bb>\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	<b> , <c> 	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/padding ()
+  (let ((delimit-columns-format 'padding)
+        (delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "<a >, <b >\n"
+                             "<aa>, <bb>\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17)  ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	<b >, <c >	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(provide 'delim-col-tests)
+;;; delim-col-tests.el ends here
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-08 20:24         ` Stefan Kangas
@ 2019-05-09 13:39           ` Basil L. Contovounesios
  2019-05-09 14:38             ` Stefan Monnier
  0 siblings, 1 reply; 12+ messages in thread
From: Basil L. Contovounesios @ 2019-05-09 13:39 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: emacs-devel

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

Stefan Kangas <stefan@marxist.se> writes:

> I just got back the confirmation for the copyright assignment.

Great.  Can someone please check that it's indeed on file?  I don't
think I have access to that information.

> If there are no more outstanding issues, I think this is ready to be
> merged.

Once the assignment is confirmed on our side as well, and if no-one
objects within the next few days, I or someone else will push the patch
to master.

> I've attached the patch (again) with a better commit message and
> proper thanks given to Basil.

Thanks, I noticed an additional possible cleanup:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: delim-col-headers.diff --]
[-- Type: text/x-diff, Size: 755 bytes --]

diff --git a/lisp/delim-col.el b/lisp/delim-col.el
index a968b32052..910178020d 100644
--- a/lisp/delim-col.el
+++ b/lisp/delim-col.el
@@ -5,8 +5,8 @@
 ;; Author: Vinicius Jose Latorre <viniciusjl.gnu@gmail.com>
 ;; Maintainer: Vinicius Jose Latorre <viniciusjl.gnu@gmail.com>
 ;; Version: 2.1
-;; Keywords: internal
-;; X-URL: http://www.emacswiki.org/cgi-bin/wiki/ViniciusJoseLatorre
+;; Keywords: convenience text
+;; X-URL: https://www.emacswiki.org/emacs/ViniciusJoseLatorre
 
 ;; This file is part of GNU Emacs.
 
@@ -125,6 +125,7 @@ columns
   "Prettify columns."
   :link '(emacs-library-link :tag "Source Lisp File" "delim-col.el")
   :prefix "delimit-columns-"
+  :group 'convenience
   :group 'text)
 
 (defcustom delimit-columns-str-before ""

[-- Attachment #3: Type: text/plain, Size: 37 bytes --]


Any objections?

Thanks,

-- 
Basil

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-09 13:39           ` Basil L. Contovounesios
@ 2019-05-09 14:38             ` Stefan Monnier
  2019-05-11 10:46               ` Stefan Kangas
  0 siblings, 1 reply; 12+ messages in thread
From: Stefan Monnier @ 2019-05-09 14:38 UTC (permalink / raw)
  To: emacs-devel

>> I just got back the confirmation for the copyright assignment.
> Great.  Can someone please check that it's indeed on file?

Confirmed.

BTW, it would be nice to make it so that delimit-columns-region
delegates to delimit-columns-rectangle when called with a rectangular
region (i.e. using rectangle-mark-mode).


        Stefan




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-09 14:38             ` Stefan Monnier
@ 2019-05-11 10:46               ` Stefan Kangas
  2019-05-20 14:40                 ` Basil L. Contovounesios
  0 siblings, 1 reply; 12+ messages in thread
From: Stefan Kangas @ 2019-05-11 10:46 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel


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

Basil L. Contovounesios <contovob@tcd.ie> writes:
> if no-one objects within the next few days, I or someone else will push
the
> patch to master.

Great.

> I noticed an additional possible cleanup:

I've included these cleanups in the attached patch.

Could we also get rid of this comment?  I don't see how it helps since this
has been in vanilla for 20 years now.

;; To use it, make sure that this file is in load-path and insert in your
;; .emacs:
;;
;;    (require 'delim-col)

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> BTW, it would be nice to make it so that delimit-columns-region
> delegates to delimit-columns-rectangle when called with a rectangular
> region (i.e. using rectangle-mark-mode).

Good idea.  How about the second patch attached here?

Thanks,
Stefan Kangas

[-- Attachment #1.2: Type: text/html, Size: 1166 bytes --]

[-- Attachment #2: 0001-Use-lexical-binding-in-delim-col.el-and-add-tests-5.patch --]
[-- Type: text/x-patch, Size: 15189 bytes --]

From 4271325793992e76fb79ebfccf9644c757d86283 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 5 May 2019 15:48:57 +0200
Subject: [PATCH 1/2] Use lexical-binding in delim-col.el and add tests

Thanks to Basil L. Contovounesios for additional cleanups.

* lisp/delim-col.el: Use lexical-binding

* test/lisp/delim-col-tests.el (delim-col-tests-delimit-colummns-before-after)
(delim-col-tests-delimit-columns)
(delim-col-tests-delimit-columns-format/nil)
(delim-col-tests-delimit-columns-format/padding)
(delim-col-tests-delimit-columns-format/separator)
(delim-col-tests-delimit-columns-separator)
(delim-col-tests-delimit-columns-str-before-after)
(delim-col-tests-delimit-columns-str-separator)
(delim-col-tests-delimit-rectangle): New unit tests.
---
 lisp/delim-col.el            |  65 ++++++++--------
 test/lisp/delim-col-tests.el | 181 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 212 insertions(+), 34 deletions(-)
 create mode 100644 test/lisp/delim-col-tests.el

diff --git a/lisp/delim-col.el b/lisp/delim-col.el
index a968b32052..6904efaad7 100644
--- a/lisp/delim-col.el
+++ b/lisp/delim-col.el
@@ -1,12 +1,12 @@
-;;; delim-col.el --- prettify all columns in a region or rectangle
+;;; delim-col.el --- prettify all columns in a region or rectangle  -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 1999-2019 Free Software Foundation, Inc.
 
 ;; Author: Vinicius Jose Latorre <viniciusjl.gnu@gmail.com>
 ;; Maintainer: Vinicius Jose Latorre <viniciusjl.gnu@gmail.com>
 ;; Version: 2.1
-;; Keywords: internal
-;; X-URL: http://www.emacswiki.org/cgi-bin/wiki/ViniciusJoseLatorre
+;; Keywords: convenience text
+;; X-URL: https://www.emacswiki.org/emacs/ViniciusJoseLatorre
 
 ;; This file is part of GNU Emacs.
 
@@ -91,9 +91,9 @@
 ;;	aaa	[ <bbb>, <cccc>    ]	dddd
 ;;	aa	[ <bb> , <ccccccc> ]	ddd
 ;;
-;; Note that `delimit-columns-region' operates over all text region
-;; selected, extending the region start to the beginning of line and the
-;; region end to the end of line.  While `delimit-columns-rectangle'
+;; Note that `delimit-columns-region' operates over the entire selected
+;; text region, extending the region start to the beginning of line and
+;; the region end to the end of line.  While `delimit-columns-rectangle'
 ;; operates over the text rectangle selected which rectangle diagonal is
 ;; given by the region start and end.
 ;;
@@ -117,6 +117,7 @@
 
 ;;; Code:
 
+(require 'rect)
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; User Options:
@@ -125,6 +126,7 @@ columns
   "Prettify columns."
   :link '(emacs-library-link :tag "Source Lisp File" "delim-col.el")
   :prefix "delimit-columns-"
+  :group 'convenience
   :group 'text)
 
 (defcustom delimit-columns-str-before ""
@@ -213,10 +215,11 @@ delimit-columns-start
 The following relation must hold:
    0 <= delimit-columns-start <= delimit-columns-end
 
-The column number start from 0 and it's relative to the beginning of selected
-region.  So if you selected a text region, the first column (column 0) is
-located at beginning of line.  If you selected a text rectangle, the first
-column (column 0) is located at left corner."
+The column number starts at 0 and is relative to the beginning of
+the selected region.  So if you select a text region, the first
+column (column 0) is located at the beginning of line.  If you
+select a text rectangle, the first column (column 0) is located
+at the left corner."
   :type '(integer :tag "Column Start")
   :group 'columns)
 
@@ -228,10 +231,11 @@ delimit-columns-end
 The following relation must hold:
    0 <= delimit-columns-start <= delimit-columns-end
 
-The column number start from 0 and it's relative to the beginning of selected
-region.  So if you selected a text region, the first column (column 0) is
-located at beginning of line.  If you selected a text rectangle, the first
-column (column 0) is located at left corner."
+The column number starts at 0 and is relative to the beginning of
+the selected region.  So if you select a text region, the first
+column (column 0) is located at the beginning of line.  If you
+select a text rectangle, the first column (column 0) is located
+at the left corner."
   :type '(integer :tag "Column End")
   :group 'columns)
 
@@ -247,20 +251,20 @@ delimit-columns-limit
 
 ;;;###autoload
 (defun delimit-columns-customize ()
-  "Customization of `columns' group."
+  "Customize the `columns' group."
   (interactive)
   (customize-group 'columns))
 
 
-(defmacro delimit-columns-str (str)
-  `(if (stringp ,str) ,str ""))
+(defun delimit-columns-str (str)
+  (if (stringp str) str ""))
 
 
 ;;;###autoload
 (defun delimit-columns-region (start end)
   "Prettify all columns in a text region.
 
-START and END delimits the text region."
+START and END delimit the text region."
   (interactive "*r")
   (let ((delimit-columns-str-before
 	 (delimit-columns-str delimit-columns-str-before))
@@ -273,8 +277,7 @@ delimit-columns-region
 	(delimit-columns-after
 	 (delimit-columns-str delimit-columns-after))
 	(delimit-columns-start
-	 (if (and (integerp delimit-columns-start)
-		  (>= delimit-columns-start 0))
+         (if (natnump delimit-columns-start)
 	     delimit-columns-start
 	   0))
 	(delimit-columns-end
@@ -309,14 +312,11 @@ delimit-columns-region
 	(set-marker the-end nil)))))
 
 
-(require 'rect)
-
-
 ;;;###autoload
 (defun delimit-columns-rectangle (start end)
   "Prettify all columns in a text rectangle.
 
-START and END delimits the corners of text rectangle."
+START and END delimit the corners of the text rectangle."
   (interactive "*r")
   (let ((delimit-columns-str-before
 	 (delimit-columns-str delimit-columns-str-before))
@@ -329,8 +329,7 @@ delimit-columns-rectangle
 	(delimit-columns-after
 	 (delimit-columns-str delimit-columns-after))
 	(delimit-columns-start
-	 (if (and (integerp delimit-columns-start)
-		  (>= delimit-columns-start 0))
+         (if (natnump delimit-columns-start)
 	     delimit-columns-start
 	   0))
 	(delimit-columns-end
@@ -344,11 +343,11 @@ delimit-columns-rectangle
       ;; get maximum length for each column
       (and delimit-columns-format
 	   (save-excursion
-	     (operate-on-rectangle 'delimit-columns-rectangle-max
+             (operate-on-rectangle #'delimit-columns-rectangle-max
 				   start the-end nil)))
       ;; prettify columns
       (save-excursion
-	(operate-on-rectangle 'delimit-columns-rectangle-line
+        (operate-on-rectangle #'delimit-columns-rectangle-line
 			      start the-end nil))
       ;; nullify markers
       (set-marker delimit-columns-limit nil)
@@ -359,7 +358,7 @@ delimit-columns-rectangle
 ;; Internal Variables and Functions:
 
 
-(defun delimit-columns-rectangle-max (startpos &optional _ignore1 _ignore2)
+(defun delimit-columns-rectangle-max (startpos &optional _begextra _endextra)
   (set-marker delimit-columns-limit (point))
   (goto-char startpos)
   (let ((ncol 1)
@@ -392,7 +391,7 @@ delimit-columns-rectangle-max
       (setq values (cdr values)))))
 
 
-(defun delimit-columns-rectangle-line (startpos &optional _ignore1 _ignore2)
+(defun delimit-columns-rectangle-line (startpos &optional _begextra _endextra)
   (let ((len  (length delimit-columns-max))
 	(ncol 0)
 	origin)
@@ -442,8 +441,7 @@ delimit-columns-rectangle-line
 	    ((eq delimit-columns-format 'padding)
 	     (insert spaces delimit-columns-after delimit-columns-str-after))
 	    (t
-	     (insert delimit-columns-after spaces delimit-columns-str-after))
-	    ))
+             (insert delimit-columns-after spaces delimit-columns-str-after))))
     (goto-char (max (point) delimit-columns-limit))))
 
 
@@ -466,8 +464,7 @@ delimit-columns-format
 	 (insert delimit-columns-after
 		 delimit-columns-str-separator
 		 spaces
-		 delimit-columns-before))
-	))
+                 delimit-columns-before))))
 
 \f
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
diff --git a/test/lisp/delim-col-tests.el b/test/lisp/delim-col-tests.el
new file mode 100644
index 0000000000..f2a0377b07
--- /dev/null
+++ b/test/lisp/delim-col-tests.el
@@ -0,0 +1,181 @@
+;;; delim-col-tests.el --- Tests for delim-col.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2019 Free Software Foundation, Inc.
+
+;; Author: Stefan Kangas <stefankangas@gmail.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'delim-col)
+
+(ert-deftest delim-col-tests-delimit-columns ()
+  (with-temp-buffer
+    (insert "a	b	c\n")
+    (delimit-columns-region (point-min) (point-max))
+    (should (equal (buffer-string) "a, b, c\n")))
+  (with-temp-buffer
+    (insert "a	b	c	d\n"
+            "aaaa	bb	ccc	ddddd\n"
+            "aaa	bbb	cccc	dddd\n"
+            "aa	bb	ccccccc	ddd\n")
+    (delimit-columns-region (point-min) (point-max))
+    (should (equal (buffer-string)
+                   (concat "a,    b,   c,       d    \n"
+                           "aaaa, bb,  ccc,     ddddd\n"
+                           "aaa,  bbb, cccc,    dddd \n"
+                           "aa,   bb,  ccccccc, ddd  \n")))))
+
+(ert-deftest delim-col-tests-delimit-rectangle ()
+  (with-temp-buffer
+    (insert "a	b	c	d\n"
+            "aaaa	bb	ccc	ddddd\n"
+            "aaa	bbb	cccc	dddd\n"
+            "aa	bb	ccccccc	ddd\n")
+    (delimit-columns-rectangle 3 58) ; from first b to last c
+    (should (equal (buffer-string)
+                   (concat "a	b,   c      	d\n"
+                           "aaaa	bb,  ccc    	ddddd\n"
+                           "aaa	bbb, cccc   	dddd\n"
+                           "aa	bb,  ccccccc	ddd\n")))))
+
+(ert-deftest delim-col-tests-delimit-columns-str-separator ()
+  (let ((delimit-columns-str-separator ":"))
+    (with-temp-buffer
+      (insert "a	b\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "a:b\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 16) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	b: c	d\n"
+                             "aa	bb:cc	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-str-before-after ()
+  (let ((delimit-columns-str-before "[ ")
+        (delimit-columns-str-after " ]"))
+    (with-temp-buffer
+      (insert "a	b	c\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "[ a, b, c ]\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aaaa	bb	ccc	ddddd\n"
+              "aaa	bbb	cccc	dddd\n"
+              "aa	bb	ccccccc	ddd\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "[ a,    b,   c,       d     ]\n"
+                             "[ aaaa, bb,  ccc,     ddddd ]\n"
+                             "[ aaa,  bbb, cccc,    dddd  ]\n"
+                             "[ aa,   bb,  ccccccc, ddd   ]\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aaaa	bb	ccc	ddddd\n"
+              "aaa	bbb	cccc	dddd\n"
+              "aa	bb	ccccccc	ddd\n")
+     (delimit-columns-rectangle 3 58)   ; from first b to last c
+     (should (equal (buffer-string)
+                    (concat "a	[ b,   c       ]	d\n"
+                            "aaaa	[ bb,  ccc     ]	ddddd\n"
+                            "aaa	[ bbb, cccc    ]	dddd\n"
+                            "aa	[ bb,  ccccccc ]	ddd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-colummns-before-after ()
+  (let ((delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "<a>, <b>\n")))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17)
+      (should (equal (buffer-string)
+                     (concat "a	<b>,  <c> 	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-separator ()
+  (let ((delimit-columns-separator ","))
+    (with-temp-buffer
+      (insert "a,b,c\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string) "a, b, c\n")))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/nil ()
+  (let ((delimit-columns-format nil))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "a, b\n"
+                             "aa, bb\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	b, c	d\n"
+                             "aa	bb, cc	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/separator ()
+  (let ((delimit-columns-format 'separator)
+        (delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "<a> , <b> \n"
+                             "<aa>, <bb>\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17) ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	<b> , <c> 	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(ert-deftest delim-col-tests-delimit-columns-format/padding ()
+  (let ((delimit-columns-format 'padding)
+        (delimit-columns-before "<")
+        (delimit-columns-after ">"))
+    (with-temp-buffer
+      (insert "a	b\n"
+              "aa	bb\n")
+      (delimit-columns-region (point-min) (point-max))
+      (should (equal (buffer-string)
+                     (concat "<a >, <b >\n"
+                             "<aa>, <bb>\n"))))
+    (with-temp-buffer
+      (insert "a	b	c	d\n"
+              "aa	bb	cc	dd\n")
+      (delimit-columns-rectangle 3 17)  ; from first b to last c
+      (should (equal (buffer-string)
+                     (concat "a	<b >, <c >	d\n"
+                             "aa	<bb>, <cc>	dd\n"))))))
+
+(provide 'delim-col-tests)
+;;; delim-col-tests.el ends here
-- 
2.11.0


[-- Attachment #3: 0002-Delegate-to-rectangle-version-in-delim-col-when-appr.patch --]
[-- Type: text/x-patch, Size: 4569 bytes --]

From 5b2ef61a06001c48a414df3c63f2588c18038f90 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sat, 11 May 2019 12:17:18 +0200
Subject: [PATCH 2/2] Delegate to rectangle version in delim-col when
 appropriate

* lisp/delim-col.el (delimit-columns-region): Delegate to
delimit-columns-rectangle when called with a rectangular region.
---
 etc/NEWS          |  6 ++++
 lisp/delim-col.el | 92 +++++++++++++++++++++++++++++--------------------------
 2 files changed, 54 insertions(+), 44 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index d10a553244..fb70de7902 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1422,6 +1422,12 @@ of an idle Emacs, but may fail on some network file systems; set
 notification is not supported.  The new variable currently has no
 effect in 'global-auto-revert-mode'.  The default value is nil.
 
+** delim-col
+
+*** 'delimit-columns-region' now delegates to
+'delimit-columns-rectangle' when called with a rectangular region
+(i.e. using rectangle-mark-mode).
+
 \f
 * New Modes and Packages in Emacs 27.1
 
diff --git a/lisp/delim-col.el b/lisp/delim-col.el
index 6904efaad7..a3ba1883af 100644
--- a/lisp/delim-col.el
+++ b/lisp/delim-col.el
@@ -266,50 +266,54 @@ delimit-columns-region
 
 START and END delimit the text region."
   (interactive "*r")
-  (let ((delimit-columns-str-before
-	 (delimit-columns-str delimit-columns-str-before))
-	(delimit-columns-str-separator
-	 (delimit-columns-str delimit-columns-str-separator))
-	(delimit-columns-str-after
-	 (delimit-columns-str delimit-columns-str-after))
-	(delimit-columns-before
-	 (delimit-columns-str delimit-columns-before))
-	(delimit-columns-after
-	 (delimit-columns-str delimit-columns-after))
-	(delimit-columns-start
-         (if (natnump delimit-columns-start)
-	     delimit-columns-start
-	   0))
-	(delimit-columns-end
-	 (if (integerp delimit-columns-end)
-	     delimit-columns-end
-	   1000000))
-	(delimit-columns-limit (make-marker))
-	(the-end (copy-marker end))
-	delimit-columns-max)
-    (when (<= delimit-columns-start delimit-columns-end)
-      (save-excursion
-	(goto-char start)
-	(beginning-of-line)
-	;; get maximum length for each column
-	(and delimit-columns-format
-	     (save-excursion
-	       (while (< (point) the-end)
-		 (delimit-columns-rectangle-max
-		  (prog1
-		      (point)
-		    (end-of-line)))
-		 (forward-char 1))))
-	;; prettify columns
-	(while (< (point) the-end)
-	  (delimit-columns-rectangle-line
-	   (prog1
-	       (point)
-	     (end-of-line)))
-	  (forward-char 1))
-	;; nullify markers
-	(set-marker delimit-columns-limit nil)
-	(set-marker the-end nil)))))
+  (if rectangle-mark-mode
+      ;; Delegate to delimit-columns-rectangle when called with a
+      ;; rectangular region.
+      (delimit-columns-rectangle start end)
+    (let ((delimit-columns-str-before
+           (delimit-columns-str delimit-columns-str-before))
+          (delimit-columns-str-separator
+           (delimit-columns-str delimit-columns-str-separator))
+          (delimit-columns-str-after
+           (delimit-columns-str delimit-columns-str-after))
+          (delimit-columns-before
+           (delimit-columns-str delimit-columns-before))
+          (delimit-columns-after
+           (delimit-columns-str delimit-columns-after))
+          (delimit-columns-start
+           (if (natnump delimit-columns-start)
+               delimit-columns-start
+             0))
+          (delimit-columns-end
+           (if (integerp delimit-columns-end)
+               delimit-columns-end
+             1000000))
+          (delimit-columns-limit (make-marker))
+          (the-end (copy-marker end))
+          delimit-columns-max)
+      (when (<= delimit-columns-start delimit-columns-end)
+        (save-excursion
+          (goto-char start)
+          (beginning-of-line)
+          ;; get maximum length for each column
+          (and delimit-columns-format
+               (save-excursion
+                 (while (< (point) the-end)
+                   (delimit-columns-rectangle-max
+                    (prog1
+                        (point)
+                      (end-of-line)))
+                   (forward-char 1))))
+          ;; prettify columns
+          (while (< (point) the-end)
+            (delimit-columns-rectangle-line
+             (prog1
+                 (point)
+               (end-of-line)))
+            (forward-char 1))
+          ;; nullify markers
+          (set-marker delimit-columns-limit nil)
+          (set-marker the-end nil))))))
 
 
 ;;;###autoload
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-11 10:46               ` Stefan Kangas
@ 2019-05-20 14:40                 ` Basil L. Contovounesios
  2019-05-30 18:20                   ` Stefan Kangas
  0 siblings, 1 reply; 12+ messages in thread
From: Basil L. Contovounesios @ 2019-05-20 14:40 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: Stefan Monnier, emacs-devel

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

Stefan Kangas <stefan@marxist.se> writes:

> Basil L. Contovounesios <contovob@tcd.ie> writes:
>> if no-one objects within the next few days, I or someone else will push the
>> patch to master.
>
> Great.
>
>> I noticed an additional possible cleanup:
>
> I've included these cleanups in the attached patch.
>
> Could we also get rid of this comment?  I don't see how it helps since this
> has been in vanilla for 20 years now.
>
> ;; To use it, make sure that this file is in load-path and insert in your
> ;; .emacs:
> ;;
> ;;    (require 'delim-col)

Sure.  I added that, a link to this discussion, and a missing full stop
to your first patch, refilled one of its long lines, and pushed to
master[1].

[1: 4498e5a13a]: Use lexical-binding in delim-col.el and add tests
  2019-05-20 15:29:26 +0100
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=4498e5a13a3b63a3024ceef102ae3b5c50f58be1

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> BTW, it would be nice to make it so that delimit-columns-region
>> delegates to delimit-columns-rectangle when called with a rectangular
>> region (i.e. using rectangle-mark-mode).
>
> Good idea.  How about the second patch attached here?

Looks fine to me, except for a minor question:

> From 5b2ef61a06001c48a414df3c63f2588c18038f90 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 11 May 2019 12:17:18 +0200
> Subject: [PATCH 2/2] Delegate to rectangle version in delim-col when
>  appropriate
>
> * lisp/delim-col.el (delimit-columns-region): Delegate to
> delimit-columns-rectangle when called with a rectangular region.
> ---
>  etc/NEWS          |  6 ++++
>  lisp/delim-col.el | 92 +++++++++++++++++++++++++++++--------------------------
>  2 files changed, 54 insertions(+), 44 deletions(-)
>
> diff --git a/etc/NEWS b/etc/NEWS
> index d10a553244..fb70de7902 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1422,6 +1422,12 @@ of an idle Emacs, but may fail on some network file systems; set
>  notification is not supported.  The new variable currently has no
>  effect in 'global-auto-revert-mode'.  The default value is nil.
>  
> +** delim-col
> +
> +*** 'delimit-columns-region' now delegates to
> +'delimit-columns-rectangle' when called with a rectangular region
> +(i.e. using rectangle-mark-mode).
> +
>  \f
>  * New Modes and Packages in Emacs 27.1

I'm personally fine with this, but I think NEWS entries usually comprise
a summary on the first line, followed by an elaboration in the body.
I'm not very good at writing these, but here's one way to do that:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: delim-col-modes.diff --]
[-- Type: text/x-diff, Size: 637 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index d4638c17b1..1606d5b50e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1460,9 +1460,10 @@ buffer periodically when 'auto-revert-avoid-polling' is non-nil.
 
 ** delim-col
 
-*** 'delimit-columns-region' now delegates to
-'delimit-columns-rectangle' when called with a rectangular region
-(i.e. using rectangle-mark-mode).
+*** 'delimit-columns-region' acts differently on rectangular regions.
+When called with a noncontiguous rectangular region, i.e., when
+rectangle-mark-mode is enabled, 'delimit-columns-region' now acts like
+'delimit-columns-rectangle'.
 
 \f
 * New Modes and Packages in Emacs 27.1

[-- Attachment #3: Type: text/plain, Size: 86 bytes --]


Here's another possibility to reuse an existing NEWS entry under
"Editing Changes":


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: delim-col-editing.diff --]
[-- Type: text/x-diff, Size: 1036 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index d4638c17b1..e1213a036a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -407,7 +407,9 @@ interface that's more like functions like 'search-forward'.
 
 ---
 ** More commands support noncontiguous rectangular regions, namely
-'upcase-dwim', 'downcase-dwim', 'replace-string', 'replace-regexp'.
+'upcase-dwim', 'downcase-dwim', 'replace-string', 'replace-regexp',
+and 'delimit-columns-region'.  The latter delegates to
+'delimit-columns-rectangle' when the region is rectangular.
 
 +++
 ** When asked to visit a large file, Emacs now offers visiting it literally.
@@ -1458,12 +1460,6 @@ the new variable 'buffer-auto-revert-by-notification' to a non-nil
 value.  Auto Revert mode can use this information to avoid polling the
 buffer periodically when 'auto-revert-avoid-polling' is non-nil.
 
-** delim-col
-
-*** 'delimit-columns-region' now delegates to
-'delimit-columns-rectangle' when called with a rectangular region
-(i.e. using rectangle-mark-mode).
-
 \f
 * New Modes and Packages in Emacs 27.1
 

[-- Attachment #5: Type: text/plain, Size: 141 bytes --]


As soon as someone confirms which format is preferred or suggests a
better wording, I'll push the second patch as well.

Thanks,

-- 
Basil

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Unit tests and lexical-binding for delim-col.el
  2019-05-20 14:40                 ` Basil L. Contovounesios
@ 2019-05-30 18:20                   ` Stefan Kangas
  0 siblings, 0 replies; 12+ messages in thread
From: Stefan Kangas @ 2019-05-30 18:20 UTC (permalink / raw)
  To: Basil L. Contovounesios; +Cc: Stefan Monnier, emacs-devel

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

Basil L. Contovounesios <contovob@tcd.ie> writes:
> > Stefan Monnier <monnier@iro.umontreal.ca> writes:
> >> BTW, it would be nice to make it so that delimit-columns-region
> >> delegates to delimit-columns-rectangle when called with a rectangular
> >> region (i.e. using rectangle-mark-mode).
> >
> > Good idea.  How about the second patch attached here?

> I'm personally fine with this, but I think NEWS entries usually comprise
> a summary on the first line, followed by an elaboration in the body.
> I'm not very good at writing these, but here's one way to do that:
>
> Here's another possibility to reuse an existing NEWS entry under
> "Editing Changes":

Good thinking.  I think the version where we bundle this with other changes is
better.  But I would suggest to skip the implementation details.  See attached
patch.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-Delegate-to-rectangle-version-in-delim-col-when-appr-2.patch --]
[-- Type: application/x-patch, Size: 4539 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-05-30 18:20 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-07 20:56 [PATCH] Unit tests and lexical-binding for delim-col.el Stefan Kangas
2019-05-07 22:44 ` Basil L. Contovounesios
2019-05-07 23:03   ` Noam Postavsky
2019-05-08  7:36   ` Stefan Kangas
2019-05-08 12:00     ` Basil L. Contovounesios
2019-05-08 17:20       ` Stefan Kangas
2019-05-08 20:24         ` Stefan Kangas
2019-05-09 13:39           ` Basil L. Contovounesios
2019-05-09 14:38             ` Stefan Monnier
2019-05-11 10:46               ` Stefan Kangas
2019-05-20 14:40                 ` Basil L. Contovounesios
2019-05-30 18:20                   ` Stefan Kangas

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).