From: Ihor Radchenko <yantar92@posteo.net>
To: Francesc Rocher <francesc.rocher@gmail.com>
Cc: emacs-orgmode@gnu.org
Subject: Re: Testing issues for Ada/SPARK support in Babel
Date: Sat, 18 May 2024 09:30:27 +0000 [thread overview]
Message-ID: <87ttivtqks.fsf@localhost> (raw)
In-Reply-To: <CAKMo=hWqn4nK1x2+dSkCrXnZvCMsNJ4LUHAhFh+7xZEM2DX1yw@mail.gmail.com>
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>
prev parent reply other threads:[~2024-05-18 9:30 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-01 8:02 Ada/SPARK support in Babel Francesc Rocher
2023-12-17 14:17 ` Ihor Radchenko
2024-02-24 13:56 ` Bastien Guerry
2024-02-24 18:41 ` Francesc Rocher
2024-02-25 10:40 ` Ihor Radchenko
2024-03-10 18:44 ` Testing issues for " Francesc Rocher
2024-03-12 12:00 ` Ihor Radchenko
2024-05-13 8:10 ` Ihor Radchenko
2024-05-13 16:28 ` Francesc Rocher
2024-05-18 9:30 ` Ihor Radchenko [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ttivtqks.fsf@localhost \
--to=yantar92@posteo.net \
--cc=emacs-orgmode@gnu.org \
--cc=francesc.rocher@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.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).