Re: [PATCH] Unit tests and lexical-binding for delim-col.el

[Basil L. Contovounesios]

Stefan Kangas <address@hidden> 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?

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

Thanks,

-- 
Basil