Re: Testing issues for Ada/SPARK support in Babel

[Ihor Radchenko <yantar92@posteo.net>]

Francesc Rocher <francesc.rocher@gmail.com> writes:

> If you don't mind, I attach here the patch in its current state for
> reviewing purposes.

Sure. See my comments inline.

> +;; Author: Francesc Rocher
> +;; Maintainer: Francesc Rocher

If you can, please provide a contact email here.

> +
> +(require 'ob)
> +(require 'time-stamp)
> +
> +(org-assert-version)

Need (require 'org-macs) - it is where `org-assert-version' is defined.

> +(defvar org-babel-tangle-lang-exts)
> +(add-to-list 'org-babel-tangle-lang-exts '("ada" . "adb"))

Why not (require 'ob-tangle)?
Or, better (eval-after-load 'ob-tangle ...)

> +(defvar org-babel-temporary-directory)

You don't need it after (require 'ob) - org-babel-temporary-directory is
defined in ob-core, loaded by ob, and is thus available.

> +(defvar org-babel-default-header-args:ada '((:args . nil)
> +                                            (:cflags . nil)
> +                                            (:pflags . nil)
> +                                            (:prove . nil)
> +                                            (:template . nil)
> +                                            (:unit . nil)
> +                                            (:with . nil))
> +  "Ada/SPARK default header arguments.")
> +
> +(defconst org-babel-header-args:ada '((args . any)
> +                                      (cflags . any)
> +                                      (pflags . any)
> +                                      (prove . ((nil t)))
> +                                      (template . :any)
> +                                      (unit . :any)
> +                                      (with . nil))

What does (with . nil) is supposed to mean?
AFAIK, this does not follow the format we describe in
`org-babel-common-header-args-w-values'.

> +(defconst ob-ada-spark-template-var-name-format "ob-ada-spark-template:%s"
> +  "Format of the name of the template variables.
> +All templates must be defined in a variable with a name
> +compatible with this format. The template name is only the '%s'
> +part. From the source block, templates can be used in the
> +':template' header with the template name.

By convention, Elisp docstrings use double space to separate sentences.
Also, ' should be used with care as they may be interpreted
specially. You may have to protect them with \=' escape or use some
other formatting. Try M-x checkdoc and check out "D.6 Tips for
Documentation Strings" section of Elisp manual.

> +(defcustom org-babel-ada-spark-compile-command "gnatmake"
> +  "Command used to compile Ada/SPARK code into an executable.
> +May be either a command in the path, like gnatmake, or an
> +absolute path name.
> +
> +If using Alire 2.x, install the default native toolchain, with
> +`alr install gnat_native', and write here the path to gnatmake or

I think `...' is used incorrectly here - it is not a Elisp symbol.

> +  :safe #'stringp)

I very much doubt that setting this variable in buffer-locals is safe -
a file downloaded from internet may set this to something like
"rm -f /home;" and unsuspecting users may evaluate the code...

> +(defcustom org-babel-ada-spark-prove-command "gnatprove"
> ....
> +  :safe #'stringp)

Same here.

> +(defcustom org-babel-ada-spark-default-compile-flags
> ...
> +  :safe #'stringp)

And here.

> +(defcustom org-babel-ada-spark-default-prove-flags
> ...
> +  :safe #'stringp)

And here.

> +(defcustom org-babel-ada-spark-default-file-header
> +  (lambda ()

Why not making the value a named function?

> +(defun ob-ada-spark-replace-params-from-template (params)
> +  "Replace headers in PARAMS with headers defined in the template.
> +If a template defines a set of params, replace the values in
> +PARAMS with the values defined in the template, overwriting
> +parameters already defined in the source code block.
> +Return the new list of PARAMS, or the original list if the
> +template does not define any header or no template is used."

It is important to note that PARAMS is modified by side effect.

> +  (let* ((template-name (cdr (assq :template params)))
> +         (template (eval
> +                    (intern-soft
> +                     (format ob-ada-spark-template-var-name-format
> +                             template-name)))))
> +    (if template
> +        (progn
> +          (message "-- replacing params from template %s" template-name)

Any reason to display a message here? It is not a big deal when running
code interactively, but it may become annoying for users when code
blocks are evaluated in batches. For example during export.

> +          (mapc (lambda (p)
> +                  (assq-delete-all (car p) params))
> +                (cdr (assq :params template)))
> +          (append params (cdr (assq :params template))))
> +      params)))

Why not simply (append params ...)? When a plist has (:foo bar :foo
baz), the first value is used.

Also, while your approach works for your current value of
`org-babel-header-args:ada' that does not have any compound parameters,
it should be more future-compatible to use `org-babel-merge-params' that
knows about multi-class parameters.

> +(defun org-babel-expand-body:ada (body params)
> ...
> +                (message "-- expanding body from template %s" template-name)

Same question about message.

>  ...
> +           (setq body (string-replace (format "%s" key) (format "%s" val) body))))

`string-replace' is not yet available in Emacs 27 that Org mode still supports.

Also, do I understand correctly, that ob-ada does not support passing
lists or tables as variable values?

And it would be nice to support :prologue and :epilogue header arguments.

> +(defun org-babel-execute:ada (body params)
> +  "Execute or prove a block of Ada/SPARK code with org-babel.
> +BODY contains the Ada/SPARK source code to evaluate. PARAMS is
> +the list of source code block parameters.
> +
> +This function is called by `org-babel-execute-src-block'"
> +  (let* ((processed-params (org-babel-process-params params))

This is redundant. `org-babel-execute-src-block' calls
`org-babel-process-params' as a part of pre-processing.

> +(defun ob-ada-spark-execute (body params)
> +  "Execute or prove a block of Ada/SPARK code with org-babel.

"or prove"? Then, how is it different from `ob-ada-spark-prove'?

> +(defvar ada-skel-initial-string--backup "")
> +
> +(defun ob-ada-spark-pre-tangle-hook ()
> + ...
> +(defun ob-ada-spark-post-tangle-hook ()
> +  "This function is called just after `org-babel-tangle'.
> +Once the file has been generated, this function restores the
> +value of the header inserted into Ada/SPARK files."
> +  ;; if ada-skel-initial-string has not been inserted by ada-mode, then
> +  ;; insert the default header
> +  (if (boundp 'ada-skel-initial-string)
> +      (setq ada-skel-initial-string ada-skel-initial-string--backup)
> +    (save-excursion
> +      (goto-char (point-min))
> +      (insert (funcall org-babel-ada-spark-default-file-header)))))
> +
> +(add-hook 'org-babel-pre-tangle-hook #'ob-ada-spark-pre-tangle-hook)
> +(add-hook 'org-babel-post-tangle-hook #'ob-ada-spark-post-tangle-hook)

This is all awkward: (1) you are setting tangle hooks globally, not just
for ada source blocks; (2) moving around global variables can be
fragile.

Maybe we can instead extend ob-core to handle your needs? For example,
we can add abnormal hooks executed before `org-babel-tangle' populates
the target buffer? Or introduce a backend-specific hook like
org-babel-post-tangle-hook:ada?

> --- /dev/null
> +++ b/testing/lisp/test-ob-ada-spark.el
> ...
> +
> +;;; Code:
> +(require 'ob-core)
> +(require 'ob-ada-spark)

No need to put requires here. The test design for babel backends is that
tests are to be evaluated _only_ when the corresponding babel backend is loaded.

This is what

> +(unless (featurep 'ob-ada-spark)
> +  (signal 'missing-test-dependency "Support for Ada and SPARK code blocks"))

is for - when feature is not available, mark tests to be skipped.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>