Subject: Re: bug#55035: [PATCH] biblio: Replace template interpreter with a macro (a "compiler").

From: Arun Isaac <[email-masked]>
Date: Fri, 22 Apr 2022 14:30:33 +0530

Hi Ludo,

> This allows us to catch invalid templates at macro-expansion time and is
> more efficient.

I have never had occasion to use the skribilo bibliography system, and I
haven't actually tested this patch, but it looks neat! I only have
cosmetic and nitpicky changes to suggest.

> diff --git a/src/guile/skribilo/biblio.scm
> b/src/guile/skribilo/biblio.scm

This file only has deletions. But maybe, update copyright header in this
file as well? It would be nice if we had automated tests for unupdated
copyright headers.

> +    (define-syntax instantiate-body
> +      (lambda (s)
> +        (define (literal? id)
> +          (any (lambda (l)
> +                 (and (identifier? id)
> +                      (free-identifier=? id l)))

We should avoid single letter names like `l'. :-)

> +        (syntax-case s (literal ... or if G_)
> +          ((_ n str rest (... ...))

And `n'.

> diff --git a/tests/biblio.test b/tests/biblio.test
> new file mode 100644
> index 0000000..954d964
> --- /dev/null
> +++ b/tests/biblio.test
> @@ -0,0 +1,88 @@
> +;;; Exercise the `biblio' routines.                  -*- Scheme -*-
> +;;;
> +;;; Copyright (C) 2022 Ludovic Courtès <[email-masked]>

We should switch to the unicode copyright symbol ©, at least in new
files.

> +(test-equal "bibliography-template, simple"

The comma is slightly confusing. I'd go with "bibliography-template:
simple" or "simple bibliography-template".

> +(test-equal "bibliography-template, conditionals"

Likewise.

> +(test-equal "bibliography-template, tricky things"

Likewise.

Thanks! :-)
Arun