GNU bug report logs - #32121
Cuirass: add support for multiple inputs

Previous Next

Full log

Message #29 received at 32121 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Clément Lassieur <clement <at> lassieur.org>
Cc: 32121 <at> debbugs.gnu.org
Subject: Re: [bug#32121] [PATCH 3/5] database: Add support for database
 upgrades.
Date: Fri, 13 Jul 2018 10:47:40 +0200

Clément Lassieur <clement <at> lassieur.org> skribis:

> * Makefile.am: Copy SQL files into their data directory.
> * doc/cuirass.texi (Database schema): Document the change.
> * src/cuirass/database.scm (%package-sql-dir): New parameter.
> (db-load, db-get-version, db-set-version, get-target-version,
> get-upgrade-file, db-upgrade): New procedures.
> (db-init): Set version corresponding to the existing upgrade-n.sql files.
> (db-open): If database exists, upgrade it.
> * src/schema.sql: New file.
> * src/sql/upgrade-1.sql: New file.

Awesome!

What follows is nitpicking, but the patch otherwise LGTM!

For Makefile.am, please specify the new variables explicitly in the
commit log.

>  dist_pkgdata_DATA = src/schema.sql
> +dist_sql_DATA = src/sql/upgrade-*.sql

This won’t really work; you have to explicitly list the files (or use
$(wildcard …), but I have a slight preference for an explicit list.)

> +@section SchemaVersion
                  ^
Please add a space here…

> +This table keeps track of the schema version.  During the initialization, the

… and here s/This table/The @code{SchemaVersion} table/

> +(define (db-get-version db)

Rather ‘db-schema-version’?  Also, please consider adding docstrings to
top-level procedures.

> +(define (db-set-version db version)

Likewise: ‘db-set-schema-version’.

> +(define (get-target-version)

‘latest-db-schema-version’ maybe?  :-)

> +  (apply max
> +         (map string->number
> +              (map (cut match:substring <> 1)
> +                   (filter regexp-match?
> +                           (map (cut string-match
> +                                  "^upgrade-([0-9]+)\\.sql$" <>)
> +                                (scandir (%package-sql-dir))))))))

I think you can write it along these lines:

  (reduce max 0
          (map (compose string->number (cut match:substring <> 1))
               (filter-map (cut string-match …) (scandir …))))

> +(define (get-upgrade-file version)

‘schema-upgrade-file’?

> +(define (db-upgrade db)
> +  (do-ec (:range version (db-get-version db) (get-target-version))

I would rather avoid SRFI-42, not just because I can’t parse it ;-), but
also to maintain consistency and make the code possibly more accessible.

In this case I think we could use a simple loop or (for-each … (iota n))
and that wouldn’t be bad.

WDYT?

Thank you!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.