Re: [PATCH] Unit tests and lexical-binding for Tempo

["Basil L. Contovounesios" <contovob@tcd.ie>]

Thanks for working on this.

Federico Tedin <federicotedin@gmail.com> writes:

> Any feedback is greatly appreciated.

Just some minor comments and questions from me.

> diff --git a/lisp/tempo.el b/lisp/tempo.el
> index 28afbec0f4..66192439cc 100644
> --- a/lisp/tempo.el
> +++ b/lisp/tempo.el
> @@ -1,4 +1,4 @@
> -;;; tempo.el --- Flexible template insertion
> +;;; tempo.el --- Flexible template insertion -*- lexical-binding: t; -*-
>  
>  ;; Copyright (C) 1994-1995, 2001-2019 Free Software Foundation, Inc.
>  
> @@ -211,6 +211,9 @@ tempo-region-stop
>  (make-variable-buffer-local 'tempo-match-finder)
>  (make-variable-buffer-local 'tempo-collection)
>  (make-variable-buffer-local 'tempo-dirty-collection)
> +(make-variable-buffer-local 'tempo-named-insertions)
> +(make-variable-buffer-local 'tempo-region-start)
> +(make-variable-buffer-local 'tempo-region-stop)

Why not define these with defvar-local instead?
  
>  ;;; Functions
>  
> @@ -268,11 +271,14 @@ tempo-define-template
>   - `n>': Inserts a newline and indents line.
>   - `o': Like `%' but leaves the point before the newline.
>   - nil: It is ignored.
> - - Anything else: It is evaluated and the result is treated as an
> -   element to be inserted.  One additional tag is useful for these
> -   cases.  If an expression returns a list (l foo bar), the elements
> -   after `l' will be inserted according to the usual rules.  This makes
> -   it possible to return several elements from one expression."
> + - Anything else: Each function in `tempo-user-elements' is called
> +   with it as argument until one of them returns non-nil, and the
> +   result is inserted. If all of them return nil, it is evaluated and
                        ^^^
Missing space.

> +   the result is treated as an element to be inserted.  One additional
> +   tag is useful for these cases.  If an expression returns a list (l
> +   foo bar), the elements after `l' will be inserted according to the
> +   usual rules.  This makes it possible to return several elements
> +   from one expression."
>    (let* ((template-name (intern (concat "tempo-template-"
>  				       name)))
>  	 (command-name template-name))
> diff --git a/test/lisp/tempo-tests.el b/test/lisp/tempo-tests.el
> new file mode 100644
> index 0000000000..4ce0830700
> --- /dev/null
> +++ b/test/lisp/tempo-tests.el
> @@ -0,0 +1,229 @@
> +;;; tempo-tests.el --- Test suite for tempo.el  -*- lexical-binding: t; -*-
> +
> +;; Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +;; Author: Federico Tedin <federicotedin@gmail.com>
> +;; Keywords: tempo

No need for this; see (info "(elisp) Library Headers") and M-x
finder-list-keywords RET for valid keywords and their meanings.

> +
> +;; 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/>.
> +
> +;;; Code:
> +
> +(require 'tempo)
> +
> +(ert-deftest string-element-test ()

Shouldn't test names have a package-specific prefix?

> +  "Test a template containing a string element."
> +  (with-temp-buffer
> +    (tempo-define-template "test" '("GNU Emacs Tempo test"))
> +    (tempo-insert-template 'tempo-template-test nil)
> +    (should (equal (buffer-string) "GNU Emacs Tempo test"))))
> +
> +(ert-deftest p-bare-element-test ()
> +  "Test a template containing a bare `p' element."
> +  (with-temp-buffer
> +    (tempo-define-template "test" '("abcde" p))
> +    (tempo-insert-template 'tempo-template-test nil)
> +    (tempo-forward-mark)
> +    (should (equal (point) 6))))
> +
> +(ert-deftest r-bare-element-test ()
> +  "Test a template containing a bare `r' element."
> +  (with-temp-buffer
> +    (tempo-define-template "test" '("abcde" r "ghijk"))
> +    (insert "F")
> +    (set-mark-command nil)

Why not set-mark?

> +    (goto-char (point-min))
> +    (tempo-insert-template 'tempo-template-test t)
> +    (should (equal (buffer-string) "abcdeFghijk"))))
> +
> +(ert-deftest p-element-test ()
> +  "Testing template containing a `p' (prompt) element."
> +  (with-temp-buffer
> +    (tempo-define-template "test" '("hello " (p ">")))
> +    (setq tempo-interactive t)

AFAICT this changes the user option's global value, which both normal
programs and tests should avoid doing; see (info "(ert) Tests and Their
Environment").  Just let-bind instead.

> +    (cl-letf (((symbol-function 'read-string) (lambda (_) "world")))

Before calling cl-letf you need (eval-when-compile (require 'cl-lib)).
Also, shouldn't the lambda accept (&rest _) args?

> +      (tempo-insert-template 'tempo-template-test nil))
> +    (should (equal (buffer-string) "hello world"))))
> +
> +(ert-deftest P-element-test ()
> +  "Testing template containing a `P' (prompt) element."
> +  (with-temp-buffer
> +    (tempo-define-template "test" '("hello " (P ">")))
> +    (setq tempo-interactive nil) ;; `P' will ignore this

RHS comments are usually written with a single semicolon;
see (info "(elisp) Comment Tips").

> +    (cl-letf (((symbol-function 'read-string) (lambda (_) "world")))
> +      (tempo-insert-template 'tempo-template-test nil))
> +    (should (equal (buffer-string) "hello world"))))

[...]

> +(ert-deftest tempo-user-elements-test ()
> +  "Testing a template containing an element to be used as an argument
> +in a call to a function in `tempo-user-elements'."
> +  (with-temp-buffer
> +    (make-variable-buffer-local 'tempo-user-elements)

This makes tempo-user-elements automatically buffer-local globally.
I think you're after make-local-variable instead.

> +    (add-to-list 'tempo-user-elements (lambda (x) (int-to-string (* x x))))
> +    (tempo-define-template "test" '(1 " " 2 " " 3 " " 4))
> +    (tempo-insert-template 'tempo-template-test nil)
> +    (should (equal (buffer-string) "1 4 9 16"))))
> +
> +(ert-deftest expand-tag-test ()
> +  "Testing expansion of a template with a tag."
> +  (with-temp-buffer
> +    (tempo-define-template "test" '("Hello, World!") "hello")
> +    (insert "hello")
> +    (tempo-complete-tag)
> +    (should (equal (buffer-string) "Hello, World!"))))
> +
> +(ert-deftest expand-partial-tag-test ()
> +  "Testing expansion of a template with a tag, with a partial match."
> +  (with-temp-buffer
> +    (tempo-define-template "test" '("Hello, World!") "hello")
> +    (insert "hel")
> +    (tempo-complete-tag)
> +    (should (equal (buffer-string) "Hello, World!"))))
> +
> +(provide 'tempo-tests)
> +;;; tempo-tests.el ends here

-- 
Basil