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?