From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45144) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fdtkD-0008Vt-Jk for guix-patches@gnu.org; Fri, 13 Jul 2018 04:48:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fdtkA-0006XW-HY for guix-patches@gnu.org; Fri, 13 Jul 2018 04:48:05 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:49777) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fdtkA-0006XK-Az for guix-patches@gnu.org; Fri, 13 Jul 2018 04:48:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1fdtkA-0001ix-4B for guix-patches@gnu.org; Fri, 13 Jul 2018 04:48:02 -0400 Subject: [bug#32121] [PATCH 3/5] database: Add support for database upgrades. Resent-Message-ID: From: ludo@gnu.org (Ludovic =?UTF-8?Q?Court=C3=A8s?=) References: <20180710230247.16639-1-clement@lassieur.org> <20180710230247.16639-3-clement@lassieur.org> Date: Fri, 13 Jul 2018 10:47:40 +0200 In-Reply-To: <20180710230247.16639-3-clement@lassieur.org> ("=?UTF-8?Q?Cl=C3=A9ment?= Lassieur"'s message of "Wed, 11 Jul 2018 01:02:45 +0200") Message-ID: <87sh4nldxv.fsf@gnu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: =?UTF-8?Q?Cl=C3=A9ment?= Lassieur Cc: 32121@debbugs.gnu.org Cl=C3=A9ment Lassieur 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 =3D src/schema.sql > +dist_sql_DATA =3D src/sql/upgrade-*.sql This won=E2=80=99t really work; you have to explicitly list the files (or u= se $(wildcard =E2=80=A6), but I have a slight preference for an explicit list.) > +@section SchemaVersion ^ Please add a space here=E2=80=A6 > +This table keeps track of the schema version. During the initialization= , the =E2=80=A6 and here s/This table/The @code{SchemaVersion} table/ > +(define (db-get-version db) Rather =E2=80=98db-schema-version=E2=80=99? Also, please consider adding d= ocstrings to top-level procedures. > +(define (db-set-version db version) Likewise: =E2=80=98db-set-schema-version=E2=80=99. > +(define (get-target-version) =E2=80=98latest-db-schema-version=E2=80=99 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 =E2=80=A6) (scandir =E2=80=A6)= ))) > +(define (get-upgrade-file version) =E2=80=98schema-upgrade-file=E2=80=99? > +(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=E2=80=99t 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 =E2=80=A6 (iot= a n)) and that wouldn=E2=80=99t be bad. WDYT? Thank you! Ludo=E2=80=99.