From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?G=C3=A1bor_Boskovits?= Subject: Re: Web interface review Date: Fri, 20 Jul 2018 10:59:26 +0200 Message-ID: References: <87sh4encn4.fsf@lassieur.org> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="000000000000fc2faa05716a8358" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:42810) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fgRGL-0007Rg-3G for guix-devel@gnu.org; Fri, 20 Jul 2018 04:59:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fgRGG-00060F-1N for guix-devel@gnu.org; Fri, 20 Jul 2018 04:59:45 -0400 Received: from mail-it0-x234.google.com ([2607:f8b0:4001:c0b::234]:37305) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fgRGF-0005yr-Iq for guix-devel@gnu.org; Fri, 20 Jul 2018 04:59:39 -0400 Received: by mail-it0-x234.google.com with SMTP id p17-v6so13655787itc.2 for ; Fri, 20 Jul 2018 01:59:39 -0700 (PDT) In-Reply-To: <87sh4encn4.fsf@lassieur.org> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: =?UTF-8?Q?Cl=C3=A9ment_Lassieur?= Cc: Guix-devel , Tatiana Sholokhova --000000000000fc2faa05716a8358 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cl=C3=A9ment Lassieur ezt =C3=ADrta (id=C5=91pont: 2= 018. j=C3=BAl. 19., Cs, 23:23): > Hello Tatiana, > > While your commits aren't pushed to master, they can be modified as much > as you want. It's good to take advantage of this to make them very > consistent. For example, they shouldn't correct one another. The > naming, also, should describe what they do. It would be inconvenient to > add the huge static files in the commits where you do the actual work, > so maybe we can set them appart. It's also good that after each commit, > Cuirass is still in a working and consistent state. > > What do you think of: > > 1. One commit to add all the static files. > 2. One commit to add the whole web interface. > > I'm not sure about it, though. Usually it's better if the commits are > short, but it's also difficult to split the work into small parts > consistenly. Don't hesitate to tell me if you find a better alternative > :-). > > What I usually do here is that I work on two branches, on one branch I keep small separate commits to aid development, and on the other, which is the one I actually push I squash the commits belonging together. Also I usually have my branch with small commits in a repo, so it is easier to review. I'll be reviewing the "second commit": everything except the static > files. But just one note about the static files: they need to be > declared in the Makefile, so that they are copied to their right > location. That would look like: > > --8<---------------cut here---------------start------------->8--- > diff --git a/Makefile.am b/Makefile.am > index f519f51..549713a 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -34,6 +34,10 @@ pkgobjectdir =3D $(guileobjectdir)/$(PACKAGE) > webmoduledir =3D $(guilesitedir)/web/server > webobjectdir =3D $(guileobjectdir)/web/server > sqldir =3D $(pkgdatadir)/sql > +staticdir =3D $(pkgdatadir)/static > +cssdir =3D $(staticdir)/css > +fontsdir =3D $(staticdir)/fonts > +imagesdir =3D $(staticdir)/images > > dist_pkgmodule_DATA =3D \ > src/cuirass/base.scm \ > @@ -62,6 +66,18 @@ dist_pkgdata_DATA =3D src/schema.sql > dist_sql_DATA =3D \ > src/sql/upgrade-1.sql > > +dist_css_DATA =3D \ > + src/static/css/bootstrap.css \ > + src/static/css/open-iconic-bootstrap.css > +dist_fonts_DATA =3D \ > + src/static/fonts/open-iconic.eot \ > + src/static/fonts/open-iconic.otf \ > + src/static/fonts/open-iconic.svg \ > + src/static/fonts/open-iconic.ttf \ > + src/static/fonts/open-iconic.woff > +dist_images_DATA =3D \ > + src/static/images/logo.png > + > TEST_EXTENSIONS =3D .scm .sh > AM_TESTS_ENVIRONMENT =3D \ > env GUILE_AUTO_COMPILE=3D'0' \ > --8<---------------cut here---------------end--------------->8--- > > Could you add a copyright header at the beginning of database.scm? > > > ;; XXX Change caller and remove > > (define (assqx-ref filters key) > > @@ -533,17 +539,27 @@ Assumes that if group id stays the same the group > headers stay the same." > > (collect-outputs x-builds-id x-repeated-row '() rows))))) > > > > (let* ((order (match (assq 'order filters) > > - (('order 'build-id) "Builds.id ASC") > > - (('order 'decreasing-build-id) "Builds.id DESC") > > - (('order 'finish-time) "Builds.stoptime DESC") > > - (('order 'start-time) "Builds.starttime DESC") > > - (('order 'submission-time) "Builds.timestamp DESC") > > + ;(('order 'build-id) "Builds.id ASC") > > + ;(('order 'decreasing-build-id) "Builds.id DESC") > > + ;(('order 'finish-time) "Builds.stoptime DESC") > > + ;(('order 'start-time) "Builds.starttime DESC") > > + ;(('order 'submission-time) "Builds.timestamp DESC") > > + ;(('order 'status+submission-time) > ^ > Could you please remove the code comments? (And in other places too :-)) > > > + (('order 'build-id) "id ASC") > ^ > And fix the indentation :-) > > About the indentation, I've seen it at several places in the code. > Maybe you could try to setup your text editor so that it indents Scheme > correctly. Don't hesitate to ask for help about it. You can also join > us on IRC (#guix), we'd happily share our tricks. > > > + (('order 'decreasing-build-id) "id DESC") > > + (('order 'finish-time) "stoptime DESC") > > + (('order 'start-time) "starttime DESC") > > + (('order 'submission-time) "timestamp DESC") > > [...] > > > @@ -624,3 +648,67 @@ FROM Evaluations ORDER BY id DESC LIMIT " limit > ";")) > > (#:specification . ,specification) > > (#:commits . ,(string-tokenize commits))) > > evaluations)))))) > > + > ^ > extra line :-) > > > +(define (db-get-evaluations-build-summary db spec limit border-low > border-high) > > + (let loop ((rows (sqlite-exec db > ^ > The string needs to start here, otherwise you'll have an indentation > issue. > > > +"SELECT E.id, E.revision, B.succeeded, B.failed, B.scheduled FROM > ^ > Please end each line with '\', so that new lines aren't interpreted by > SQLite. > > > + (SELECT id, evaluation, SUM(status=3D0) as succeeded, SUM(status>0) = as > failed, SUM(status<0) as scheduled > > + FROM Builds > > + GROUP BY evaluation) B > > + JOIN > > + (SELECT id, revision > > + FROM Evaluations > > + WHERE (specification=3D" spec ")\ > > + AND (" border-low "IS NULL OR (id >" border-low "))\ > > + AND (" border-high "IS NULL OR (id <" border-high "))\ > > + ORDER BY CASE WHEN " border-low "IS NULL THEN id ELSE -id END DESC > > + LIMIT " limit ") E > > +ON B.evaluation=3DE.id > > +ORDER BY E.id ASC;")) > > + (evaluations '())) > > + (match rows > > + (() evaluations) > > + ((#(id revision succeeded failed scheduled) > > + . rest) > > + (loop rest > > + (cons `((#:id . ,id) > > + (#:revision . ,revision) > > + (#:succeeded . ,succeeded) > > + (#:failed . ,failed) > > + (#:scheduled . ,scheduled)) > > + evaluations)))))) > > + > > +(define (db-get-evaluations-count db spec) > > + "Return the number of evaluations of the given specification SPEC" > > Our convention is that we end our sentences with a '.' in docstrings :-) > > > + (let ((rows (sqlite-exec db > > +"SELECT COUNT(id) FROM Evaluations > > +WHERE specification=3D" spec))) > > + (array-ref (list-ref rows 0) 0))) > > + > > +(define (db-get-evaluations-id-max db spec) > > + "Return the max id of evaluations of the given specification SPEC" > > + (let ((rows (sqlite-exec db > > +"SELECT MAX(id) FROM Evaluations > > +WHERE specification=3D" spec))) > > + (array-ref (list-ref rows 0) 0))) > > + > > +(define (db-get-evaluations-id-min db spec) > > + "Return the min id of evaluations of the given specification SPEC" > > + (let ((rows (sqlite-exec db > > +"SELECT MIN(id) FROM Evaluations > > +WHERE specification=3D" spec))) > > + (array-ref (list-ref rows 0) 0))) > > + > ^ extra line > > + > > +(define (db-get-builds-id-max db eval) > > + (let ((rows (sqlite-exec db > > +"SELECT MAX(stoptime) FROM Builds > > +WHERE evaluation=3D" eval))) > > + (array-ref (list-ref rows 0) 0))) > > + > ^ extra line > > + > > +(define (db-get-builds-id-min db eval) > > + (let ((rows (sqlite-exec db > > +"SELECT MIN(stoptime) FROM Builds > > +WHERE evaluation=3D" eval))) > > + (array-ref (list-ref rows 0) 0))) > > diff --git a/src/cuirass/http.scm b/src/cuirass/http.scm > > index a45e6b1..1995abf 100644 > > --- a/src/cuirass/http.scm > > +++ b/src/cuirass/http.scm > > @@ -1,8 +1,10 @@ > > + > > ;;;; http.scm -- HTTP API > > ;;; Copyright =C2=A9 2016 Mathieu Lirzin > > ;;; Copyright =C2=A9 2017 Mathieu Othacehe > > ;;; Copyright =C2=A9 2018 Ludovic Court=C3=A8s > > ;;; Copyright =C2=A9 2018 Cl=C3=A9ment Lassieur > > +;;; Copyright =C2=A9 2018 Tatiana Sholokhova > > ;;; > > ;;; This file is part of Cuirass. > > ;;; > > @@ -23,8 +25,10 @@ > > #:use-module (cuirass database) > > #:use-module (cuirass utils) > > #:use-module (cuirass logging) > > + #:use-module (srfi srfi-1) > > #:use-module (srfi srfi-11) > > #:use-module (srfi srfi-26) > > + #:use-module (ice-9 binary-ports) > > #:use-module (ice-9 match) > > #:use-module (json) > > #:use-module (web request) > > @@ -33,8 +37,39 @@ > > #:use-module (web uri) > > #:use-module (fibers) > > #:use-module (fibers channels) > > + #:use-module (sxml simple) > > + #:use-module (cuirass templates) > > #:export (run-cuirass-server)) > > > > +(define %static-directory > > + ;; Define to the static file directory. > > + (string-append (or (getenv "CUIRASS_DATADIR") > > + (string-append %datadir "/" %package)) > > + "/static")) > > Here you should use a parameter[1], it will allow %static-directory to > be "dynamically bound"[2], which makes it easy to change the setting for > a particular bit of code, restoring to its original value when done. > Plus, they are per-thread, unlike global variables. There are examples > of paremeters in database.scm. Then to access the value of the > parameter, you need to use (%static-directory) instead of > %static-directory. > > [1]: https://www.gnu.org/software/guile/manual/html_node/Parameters.html > [2]: > https://www.gnu.org/software/guile/manual/html_node/Lexical-Scope.html > > > +(define file-mime-types > > + '(("css" . (text/css)) > > + ("otf" . (font/otf)) > > + ("woff" . (font/woff)) > > + ("js" . (text/javascript)) > > + ("png" . (image/png)) > > + ("gif" . (image/gif)) > > + ("html" . (text/html)))) > > + > > +(define file-white-list > > + '("css/bootstrap.css" > > + "css/open-iconic-bootstrap.css" > > + "fonts/open-iconic.otf" > > + "fonts/open-iconic.woff" > > + "images/logo.png")) > > + > ^ extra line :-) > > + > > +(define (file-extension file-name) > > + (last (string-split file-name #\.))) > > This would behave inconsistenly if there is no extension. Please use > 'file-extension' from (guix utils). > > > +(define (directory? filename) > > + (string=3D? filename (dirname filename))) > > This doesn't work, it seems to always return #f. Instead, you can use > 'file-is-directory?' from the module (guix build union). > > As a general rule, don't hesitate to use stuff from Guix, because it's > already a dependency, it's very close to Cuirass, and it's good to avoid > copying code. Also, don't hesitate to 'grep' for stuff like > "is-directory" in the Guix repo, to find ideas. It's big and there are > many examples that you can use. > > > (define (build->hydra-build build) > > "Convert BUILD to an assoc list matching hydra API format." > > (define (bool->int bool) > > @@ -99,11 +134,12 @@ Hydra format." > > (match key-symbol > > ('id (string->number param)) > > ('nr (string->number param)) > > + ('page (string->number param)) > > (_ param))))))) > > (string-split query #\&)) > > '()))) > > > > - > > + > ^ > Here you removed the page break (^L) :-) > > > ;;; > > ;;; Web server. > > ;;; > > @@ -112,9 +148,15 @@ Hydra format." > > ;;; https://github.com/NixOS/hydra/blob/master/doc/manual/api.xml > > ;;; > > > > + > ^ > Here you added a line. > > > (define (request-path-components request) > > (split-and-decode-uri-path (uri-path (request-uri request)))) > > > > +(define (normalize-parameter parameter) > > + (if parameter > > + (list-ref parameter 0) > > + #f)) > > You don't need this function, see below. > > > (define (url-handler request body db-channel) > > > > (define* (respond response #:key body (db-channel db-channel)) > > @@ -136,6 +178,24 @@ Hydra format." > > (object->json-string > > `((error . ,message))))) > > [...] > > > + (("jobset" name) > > + (let* ((evaluation-id-max (with-critical-section db-channel (db) > (db-get-evaluations-id-max db name))) > > + (evaluation-id-min (with-critical-section db-channel (db) > (db-get-evaluations-id-min db name))) > > Could you please stick to 80 columns? I've noticed this at other places > too. > > > + (params (request-parameters request)) > > + (border-high (normalize-parameter (assq-ref params 'border-high)= )) > > + (border-low (normalize-parameter (assq-ref params 'border-low)))= ) > > We need to rewrite request-parameters so that it returns an assoc-list, > but in the meantime it's better to use the 'assqx-ref' function from > database.scm. You probably need to get it out of 'db-get-builds' > though, and to export it. > > > + (respond-html > > + (html-page > > + name > > + (evaluation-info-table > > + name > > + (with-critical-section db-channel (db) > > + (db-get-evaluations-build-summary > > + db > > + name > > + PAGESIZE > > + border-low > > + border-high > > + )) > > Please avoid dangling parentheses :-) > > > + evaluation-id-min > > + evaluation-id-max))))) > > + > > + (("eval" id) > > + (let* ((builds-id-max (with-critical-section db-channel (db) > (db-get-builds-id-max db id))) > > + (builds-id-min (with-critical-section db-channel (db) > (db-get-builds-id-min db id))) > > Here I think you can put the '(with-critical-section db-channel (db)' > around the 'let*', so that you don't need to call it twice. Same for > "jobset". > > > + (params (request-parameters request)) > > + (border-high (normalize-parameter (assq-ref params 'border-high)= )) > > + (border-low (normalize-parameter (assq-ref params 'border-low)))= ) > > + (respond-html > > + (html-page > > + "Evaluations" > > + (build-eval-table > > + (handle-builds-request db-channel > > + `((evaluation ,id) > > + (nr ,PAGESIZE) > > See below, if it's a parameter, it would be (nr ,(%pagesize)). > > > + (order finish-time) > > + (border-high ,border-high) > > + (border-low ,border-low))) > > + builds-id-min > > + builds-id-max))))) > > + > > + (("static" path ...) > > + ;(display (request-uri request)) > > + (respond-static-file path)) > > ('method-not-allowed > > ;; 405 "Method Not Allowed" > > (values (build-response #:code 405) #f db-channel)) > > (_ > > - (respond (build-response #:code 404) > > - #:body (string-append "Resource not found: " > > - (uri->string (request-uri > request))))))) > > + (respond-not-found (uri->string (request-uri request)))))) > > > > (define* (run-cuirass-server db #:key (host "localhost") (port 8080)) > > (let* ((host-info (gethostbyname host)) > > diff --git a/src/cuirass/templates.scm b/src/cuirass/templates.scm > > new file mode 100644 > > index 0000000..0e72981 > > --- /dev/null > > +++ b/src/cuirass/templates.scm > > @@ -0,0 +1,207 @@ > > + > > +;;;; http.scm -- HTTP API > > +;;; Copyright =C2=A9 2018 Tatiana Sholokhova > > +;;; > > +;;; This file is part of Cuirass. > > +;;; > > +;;; Cuirass is free software: you can redistribute it and/or modify > > +;;; it under the terms of the GNU General Public License as published = by > > +;;; the Free Software Foundation, either version 3 of the License, or > > +;;; (at your option) any later version. > > +;;; > > +;;; Cuirass is distributed in the hope that it will be useful, > > +;;; but WITHOUT ANY WARRANTY; without even the implied warranty of > > +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +;;; GNU General Public License for more details. > > +;;; > > +;;; You should have received a copy of the GNU General Public License > > +;;; along with Cuirass. If not, see . > > + > > +(define-module (cuirass templates) > > + #:export (html-page > > + specifications-table > > + build-table > > + evaluation-info-table > > + build-eval-table > > + PAGESIZE)) > > + > > +(define PAGESIZE 10) > > It's better to use Guile parameters[1] for the reasons explained above. > Also, our convention is to start global variables' names with a '%'. > That would give: > > (define %pagesize > ;; description > (make-parameter 10)) > > [1]: https://www.gnu.org/software/guile/manual/html_node/Parameters.html > > > +(define (html-page title body) > > + "Return html page with given title and body" > > + `(html (@ (xmlns "http://www.w3.org/1999/xhtml") (xml:lang "en") > (lang "en")) > > + (head > > + (meta (@ (charset "utf-8"))) > > + (meta (@ (name "viewport") > > + (content "width=3Ddevice-width, initial-scale=3D1, > shrink-to-fit=3Dno"))) > > + (link (@ (rel "stylesheet") > > + (href "/static/css/bootstrap.css"))) > > + (link (@ (rel "stylesheet") > > + (href "/static/css/open-iconic-bootstrap.css"))) > > + (title ,title)) > > + (body > > + (nav (@ (class "navbar navbar-expand-lg navbar-light bg-light")) > > + (a (@ (class "navbar-brand") (href "/")) > > + (img (@ (src "/static/images/logo.png") > > + (alt "logo") > > + (height "25"))))) > > + (main (@ (role "main") (class "container pt-4 px-1")) > > + ,body > > + (hr))))) > > + > > + > > +(define (specifications-table specs) > > + "Return body for main (Projects) html-page" > > I think it's good to refer to the arguments while describing the > function. I'd use: "Return HTML for the SPECS table." or something > similar. I think this function could be used in another context than a > "body" (please correct me if I'm wrong), so there is no need to put > "body" in the docstring. > > > + `((p (@ (class "lead")) "Projects") > > + (table > > + (@ (class "table table-sm table-hover")) > > + ,@(if (null? specs) > > + `((th (@ (scope "col")) "No elements here.")) > > + `((thead > > + (tr > > + (th (@ (scope "col")) Name) > > + (th (@ (scope "col")) Branch))) > > + (tbody > > + ,@(map > > + (lambda (spec) > > + `(tr > > + (td (a (@ (href "/jobset/" ,(assq-ref spec #:name))) > ,(assq-ref spec #:name))) > > + (td ,(assq-ref spec #:branch)))) > > + specs))))))) > > + > > +(define (pagination page-id-min page-id-max id-min id-max) > > + "Return page navigation buttons" > > + `(div (@ (class row)) > > + (nav > > + (@ (class "mx-auto") (aria-label "Page navigation")) > > + (ul (@ (class "pagination")) > > + (li (@ (class "page-item")) > > + (a (@ (class "page-link") > > + (href "?border-high=3D" ,(number->string (+ id-max > 1)))) > > + "<< First")) > > + (li (@ (class "page-item" ,(if (=3D page-id-max id-max) " > disabled" ""))) > > + (a (@ (class "page-link") > > + (href "?border-low=3D" ,(number->string page-id-max= ))) > > + "< Previous")) > > + (li (@ (class "page-item" ,(if (=3D page-id-min id-min) " > disabled" ""))) > > + (a (@ (class "page-link") > > + (href "?border-high=3D" ,(number->string page-id-mi= n))) > > + "Next >")) > > + (li (@ (class "page-item")) > > + (a (@ (class "page-link") > > + (href "?border-low=3D" ,(number->string (- id-min 1= )))) > > + "Last >>")) > > + )))) > > Could you give a status about the pagination? I have seen several > discussions about it. The goal for this commit is to have something > that we can use, but it doesn't need to be perfect. We can improve it > later. > > > +(define (minimum lst cur-min) > > I have no idea what cur-min means :-) Could you find a more descriptive > name? > > > + (cond ((null? lst) cur-min) > > + ((< (car lst) cur-min) (minimum (cdr lst) (car lst))) > > + (else (minimum (cdr lst) cur-min)))) > > + > ^ extra line :-) > > + > > +(define (maximum lst cur-max) > > + (cond ((null? lst) cur-max) > > + ((> (car lst) cur-max) (maximum (cdr lst) (car lst))) > > + (else (maximum (cdr lst) cur-max)))) > > + > ^ too > > + > > +(define (evaluation-info-table name data evaluation-id-min > evaluation-id-max) > > + "Return body for (Evaluation) html-page" > > "Return HTML for the EVALUATION table NAME from EVALUATION-ID-MIN to > EVALUATION-ID-MAX." (and please, replace 'data' with 'evaluations', it's > difficult to know what 'data' means :-)). > > > + (let ((id-min (minimum (map (lambda (row) (assq-ref row #:id)) data) > evaluation-id-max)) > > + (id-max (maximum (map (lambda (row) (assq-ref row #:id)) data) > evaluation-id-min))) > > + `((p (@ (class "lead")) "Evaluations of " ,name) > > + ;(p (@ (class "text-muted")) "Showing evaluations ",id-min > "-",id-max " out of ",evaluation-id-max) > > + (table > > + (@ (class "table table-sm table-hover table-striped")) > > + ,@(if (null? data) > > + `((th (@ (scope "col")) "No elements here.")) > > + `((thead > > + (tr > > + (th (@ (scope "col")) "#") > > + (th (@ (scope "col")) Revision) > > + (th (@ (scope "col")) Success))) > > + (tbody > > + ,@(map > > + (lambda (row) > > + `(tr > > + (th (@ (scope "row")) (a (@ (href "/eval/" > ,(assq-ref row #:id))) ,(assq-ref row #:id))) > > + (td ,(assq-ref row #:revision)) > > + (td > > + (a (@ (href "#") (class "badge badge-success")) > ,(assq-ref row #:succeeded)) > > + (a (@ (href "#") (class "badge badge-danger")) > ,(assq-ref row #:failed)) > > + (a (@ (href "#") (class "badge badge-secondary")= ) > ,(assq-ref row #:scheduled))))) > > + data))))) > > + ,(pagination id-min id-max evaluation-id-min evaluation-id-max))= )) > > + > > +(define (build-eval-table data build-id-min build-id-max) > > + > ^ extra line > > + (define (table-header) > > + `(thead > > + (tr > > + (th (@ (scope "col")) '()) > > + (th (@ (scope "col")) ID) > > + (th (@ (scope "col")) Project) > > + (th (@ (scope "col")) "Finished at") > > + (th (@ (scope "col")) Job) > > + (th (@ (scope "col")) Nixname) > > + (th (@ (scope "col")) System) > > [...] > > > +(define (build-table done pending) > > + "Return body for project's html-page" > > + (define (table-row build) > > + `(tr > > + (td ,(assq-ref build #:project)) > > + (td ,(assq-ref build #:jobset)) > > + (td ,(assq-ref build #:job)) > > + (td ,(assq-ref build #:nixname)) > > + (td ,(assq-ref build #:buildstatus)))) > > + (define (table-header) > > + `(thead > > + (tr > > + (th (@ (scope "col")) Project) > > + (th (@ (scope "col")) Jobset) > > + (th (@ (scope "col")) Job) > > + (th (@ (scope "col")) Nixname) > > + (th (@ (scope "col")) Buildstatus)))) > > + `((table > > + (@ (class "table table-sm table-hover table-striped")) > > + (caption "Latest builds") > > + ,@(if (null? done) > > + `((th (@ (scope "col")) "No elements here.")) > > + `(,(table-header) > > + (tbody > > + (@ (class "table table-sm table-hover table-striped")) > > + ,@(map table-row done))))) > > + (table > > + (@ (class "table table-sm table-hover table-striped")) > > + (caption "Queue") > > + ,@(if (null? pending) > > + `((th (@ (scope "col")) "No elements here.")) > > + `(,(table-header) > > + (tbody > > + (@ (class "table table-sm table-hover table-striped")) > > + ,@(map table-row pending))))))) > > Here I would do a procedure 'build-table' of one argument, that I would > call twice: once with the builds that are done, and once with the builds > that are pending. That would avoid the code repetition. > > Also, if you need help with git (to put the commits together), don't > hesistate to ask, either here or on IRC (#guix). > > And please, make sure 'make check' works :-). I had to replace (3 > "/baz.drv") with (1 "/foo.drv") in the "db-get-builds" test, but I > didn't look closely at it. > > And that's all! > > Thanks for all of you! Great work. As far as I can see most of the suggestions are on style. Do you think that we need some improvement in the documentation regarding style, both coding and documentation? > Thank you! > Cl=C3=A9ment > > --000000000000fc2faa05716a8358 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Cl=C3=A9ment L= assieur <clement@lassieur.org> ezt =C3=ADrta (id=C5=91pont: 2018. j=C3=BAl. 19., Cs, 23:23):
Hello Tatiana,

While your commits aren't pushed to master, they can be modified as muc= h
as you want.=C2=A0 It's good to take advantage of this to make them ver= y
consistent.=C2=A0 For example, they shouldn't correct one another.=C2= =A0 The
naming, also, should describe what they do.=C2=A0 It would be inconvenient = to
add the huge static files in the commits where you do the actual work,
so maybe we can set them appart.=C2=A0 It's also good that after each c= ommit,
Cuirass is still in a working and consistent state.

What do you think of:

=C2=A0 1. One commit to add all the static files.
=C2=A0 2. One commit to add the whole web interface.

I'm not sure about it, though.=C2=A0 Usually it's better if the com= mits are
short, but it's also difficult to split the work into small parts
consistenly.=C2=A0 Don't hesitate to tell me if you find a better alter= native
:-).


What I usually do here is that I work = on two branches, on one branch I keep
small separate commits to a= id development, and on the other, which is the one I actually push
I squash the commits belonging together. Also I usually have my branch wi= th small commits in a repo, so
I'll be reviewing the "second commit": everything except the = static
files.=C2=A0 But just one note about the static files: they need to be
declared in the Makefile, so that they are copied to their right
location.=C2=A0 That would look like:

--8<---------------cut here---------------start------------->8---
diff --git a/Makefile.am b/Makefile.am
index f519f51..549713a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -34,6 +34,10 @@ pkgobjectdir =3D $(guileobjectdir)/$(PACKAGE)
=C2=A0webmoduledir =3D $(guilesitedir)/web/server
=C2=A0webobjectdir =3D $(guileobjectdir)/web/server
=C2=A0sqldir =3D $(pkgdatadir)/sql
+staticdir =3D $(pkgdatadir)/static
+cssdir =3D $(staticdir)/css
+fontsdir =3D $(staticdir)/fonts
+imagesdir =3D $(staticdir)/images

=C2=A0dist_pkgmodule_DATA =3D=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 \
=C2=A0 =C2=A0src/cuirass/base.scm=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0\
@@ -62,6 +66,18 @@ dist_pkgdata_DATA =3D src/schema.sql
=C2=A0dist_sql_DATA =3D=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 \
=C2=A0 =C2=A0src/sql/upgrade-1.sql

+dist_css_DATA =3D=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 \
+=C2=A0 src/static/css/bootstrap.css=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0\
+=C2=A0 src/static/css/open-iconic-bootstrap.css
+dist_fonts_DATA =3D=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 \
+=C2=A0 src/static/fonts/open-iconic.eot=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0\
+=C2=A0 src/static/fonts/open-iconic.otf=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0\
+=C2=A0 src/static/fonts/open-iconic.svg=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0\
+=C2=A0 src/static/fonts/open-iconic.ttf=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0\
+=C2=A0 src/static/fonts/open-iconic.woff
+dist_images_DATA =3D=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0\
+=C2=A0 src/static/images/logo.png
+
=C2=A0TEST_EXTENSIONS =3D .scm .sh
=C2=A0AM_TESTS_ENVIRONMENT =3D \
=C2=A0 =C2=A0env GUILE_AUTO_COMPILE=3D'0' \
--8<---------------cut here---------------end--------------->8---

Could you add a copyright header at the beginning of database.scm?

>=C2=A0 =C2=A0 ;; XXX Change caller and remove
>=C2=A0 =C2=A0 (define (assqx-ref filters key)
> @@ -533,17 +539,27 @@ Assumes that if group id stays the same the grou= p headers stay the same."
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(collect-outputs x-builds-id x= -repeated-row '() rows)))))
>=C2=A0
>=C2=A0 =C2=A0 (let* ((order (match (assq 'order filters)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (('= ;order 'build-id) "Builds.id ASC")
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (('= ;order 'decreasing-build-id) "Builds.id DESC")
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (('= ;order 'finish-time) "Builds.stoptime DESC")
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (('= ;order 'start-time) "Builds.starttime DESC")
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (('= ;order 'submission-time) "Builds.timestamp DESC")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ;((= 9;order 'build-id) "Builds.id ASC")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ;((= 9;order 'decreasing-build-id) "Builds.id DESC")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ;((= 9;order 'finish-time) "Builds.stoptime DESC")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ;((= 9;order 'start-time) "Builds.starttime DESC")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ;((= 9;order 'submission-time) "Builds.timestamp DESC")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ;((= 9;order 'status+submission-time)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0^
Could you please remove the code comments?=C2=A0 (And in other places too := -))

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (('order 'build-id) "id ASC")
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0^
And fix the indentation :-)

About the indentation, I've seen it at several places in the code.
Maybe you could try to setup your text editor so that it indents Scheme
correctly.=C2=A0 Don't hesitate to ask for help about it.=C2=A0 You can= also join
us on IRC (#guix), we'd happily share our tricks.

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (('= ;order 'decreasing-build-id) "id DESC")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (('= ;order 'finish-time) "stoptime DESC")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (('= ;order 'start-time) "starttime DESC")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (('= ;order 'submission-time) "timestamp DESC")

[...]

> @@ -624,3 +648,67 @@ FROM Evaluations ORDER BY id DESC LIMIT " li= mit ";"))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0(#:specification . ,specification)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0(#:commits . ,(string-tokenize commits)))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0evaluations))))))
> +
=C2=A0 ^
extra line :-)

> +(define (db-get-evaluations-build-summary db spec limit border-low bo= rder-high)
> +=C2=A0 (let loop ((rows=C2=A0 (sqlite-exec db
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^
The string needs to start here, otherwise you'll have an indentation issue.

> +"SELECT E.id, E.revision, B.succeeded, B.failed, B.scheduled FRO= M
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0^
Please end each line with '\', so that new lines aren't interpr= eted by
SQLite.

> +=C2=A0 (SELECT id, evaluation, SUM(status=3D0) as succeeded, SUM(stat= us>0) as failed, SUM(status<0) as scheduled
> +=C2=A0 FROM Builds
> +=C2=A0 GROUP BY evaluation) B
> +=C2=A0 JOIN
> +=C2=A0 (SELECT id, revision
> +=C2=A0 FROM Evaluations
> +=C2=A0 WHERE (specification=3D" spec ")\
> +=C2=A0 AND (" border-low "IS NULL OR (id >" border-= low "))\
> +=C2=A0 AND (" border-high "IS NULL OR (id <" border= -high "))\
> +=C2=A0 ORDER BY CASE WHEN " border-low "IS NULL THEN id ELS= E -id END DESC
> +=C2=A0 LIMIT " limit ") E
> +ON B.evaluation=3DE.id
> +ORDER BY E.id ASC;"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(evaluations '())= )
> +=C2=A0 =C2=A0 (match rows
> +=C2=A0 =C2=A0 =C2=A0 (() evaluations)
> +=C2=A0 =C2=A0 =C2=A0 ((#(id revision succeeded failed scheduled)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 . rest)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(loop rest
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(cons `((#:id . ,id)<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0(#:revision . ,revision)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0(#:succeeded . ,succeeded)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0(#:failed . ,failed)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0(#:scheduled . ,scheduled))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= evaluations))))))
> +
> +(define (db-get-evaluations-count db spec)
> +=C2=A0 "Return the number of evaluations of the given specificat= ion SPEC"

Our convention is that we end our sentences with a '.' in docstring= s :-)

> +=C2=A0 (let ((rows (sqlite-exec db
> +"SELECT COUNT(id) FROM Evaluations
> +WHERE specification=3D" spec)))
> +=C2=A0 =C2=A0 (array-ref (list-ref rows 0) 0)))
> +
> +(define (db-get-evaluations-id-max db spec)
> +=C2=A0 "Return the max id of evaluations of the given specificat= ion SPEC"
> +=C2=A0 (let ((rows (sqlite-exec db
> +"SELECT MAX(id) FROM Evaluations
> +WHERE specification=3D" spec)))
> +=C2=A0 =C2=A0 (array-ref (list-ref rows 0) 0)))
> +
> +(define (db-get-evaluations-id-min db spec)
> +=C2=A0 "Return the min id of evaluations of the given specificat= ion SPEC"
> +=C2=A0 (let ((rows (sqlite-exec db
> +"SELECT MIN(id) FROM Evaluations
> +WHERE specification=3D" spec)))
> +=C2=A0 =C2=A0 (array-ref (list-ref rows 0) 0)))
> +
=C2=A0^ extra line
> +
> +(define (db-get-builds-id-max db eval)
> +=C2=A0 (let ((rows (sqlite-exec db
> +"SELECT MAX(stoptime) FROM Builds
> +WHERE evaluation=3D" eval)))
> +=C2=A0 =C2=A0 (array-ref (list-ref rows 0) 0)))
> +
=C2=A0^ extra line
> +
> +(define (db-get-builds-id-min db eval)
> +=C2=A0 (let ((rows (sqlite-exec db
> +"SELECT MIN(stoptime) FROM Builds
> +WHERE evaluation=3D" eval)))
> +=C2=A0 =C2=A0 (array-ref (list-ref rows 0) 0)))
> diff --git a/src/cuirass/http.scm b/src/cuirass/http.scm
> index a45e6b1..1995abf 100644
> --- a/src/cuirass/http.scm
> +++ b/src/cuirass/http.scm
> @@ -1,8 +1,10 @@
> +
>=C2=A0 ;;;; http.scm -- HTTP API
>=C2=A0 ;;; Copyright =C2=A9 2016 Mathieu Lirzin <mthl@gnu.org>
>=C2=A0 ;;; Copyright =C2=A9 2017 Mathieu Othacehe <m.othacehe@gmail.com>
>=C2=A0 ;;; Copyright =C2=A9 2018 Ludovic Court=C3=A8s <ludo@gnu.org>
>=C2=A0 ;;; Copyright =C2=A9 2018 Cl=C3=A9ment Lassieur <clement@lassieur.org><= br> > +;;; Copyright =C2=A9 2018 Tatiana Sholokhova <tanja201396@gmail.com>
>=C2=A0 ;;;
>=C2=A0 ;;; This file is part of Cuirass.
>=C2=A0 ;;;
> @@ -23,8 +25,10 @@
>=C2=A0 =C2=A0 #:use-module (cuirass database)
>=C2=A0 =C2=A0 #:use-module (cuirass utils)
>=C2=A0 =C2=A0 #:use-module (cuirass logging)
> +=C2=A0 #:use-module (srfi srfi-1)
>=C2=A0 =C2=A0 #:use-module (srfi srfi-11)
>=C2=A0 =C2=A0 #:use-module (srfi srfi-26)
> +=C2=A0 #:use-module (ice-9 binary-ports)
>=C2=A0 =C2=A0 #:use-module (ice-9 match)
>=C2=A0 =C2=A0 #:use-module (json)
>=C2=A0 =C2=A0 #:use-module (web request)
> @@ -33,8 +37,39 @@
>=C2=A0 =C2=A0 #:use-module (web uri)
>=C2=A0 =C2=A0 #:use-module (fibers)
>=C2=A0 =C2=A0 #:use-module (fibers channels)
> +=C2=A0 #:use-module (sxml simple)
> +=C2=A0 #:use-module (cuirass templates)
>=C2=A0 =C2=A0 #:export (run-cuirass-server))
>=C2=A0
> +(define %static-directory
> +=C2=A0 ;; Define to the static file directory.
> +=C2=A0 (string-append (or (getenv "CUIRASS_DATADIR")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0(string-append %datadir "/" %package))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"/= static"))

Here you should use a parameter[1], it will allow %static-directory to
be "dynamically bound"[2], which makes it easy to change the sett= ing for
a particular bit of code, restoring to its original value when done.
Plus, they are per-thread, unlike global variables.=C2=A0 There are example= s
of paremeters in database.scm.=C2=A0 Then to access the value of the
parameter, you need to use (%static-directory) instead of
%static-directory.

[1]: https://www.gnu.org/software= /guile/manual/html_node/Parameters.html
[2]: https://www.gnu.org/softw= are/guile/manual/html_node/Lexical-Scope.html

> +(define file-mime-types
> +=C2=A0 '(("css" . (text/css))
> +=C2=A0 =C2=A0 ("otf" . (font/otf))
> +=C2=A0 =C2=A0 ("woff" . (font/woff))
> +=C2=A0 =C2=A0 ("js"=C2=A0 . (text/javascript))
> +=C2=A0 =C2=A0 ("png" . (image/png))
> +=C2=A0 =C2=A0 ("gif" . (image/gif))
> +=C2=A0 =C2=A0 ("html" . (text/html))))
> +
> +(define file-white-list
> +=C2=A0 '("css/bootstrap.css"
> +=C2=A0 =C2=A0 "css/open-iconic-bootstrap.css"
> +=C2=A0 =C2=A0 "fonts/open-iconic.otf"
> +=C2=A0 =C2=A0 "fonts/open-iconic.woff"
> +=C2=A0 =C2=A0 "images/logo.png"))
> +
=C2=A0^ extra line :-)
> +
> +(define (file-extension file-name)
> +=C2=A0 (last (string-split file-name #\.)))

This would behave inconsistenly if there is no extension.=C2=A0 Please use<= br> 'file-extension' from (guix utils).

> +(define (directory? filename)
> +=C2=A0 (string=3D? filename (dirname filename)))

This doesn't work, it seems to always return #f.=C2=A0 Instead, you can= use
'file-is-directory?' from the module (guix build union).

As a general rule, don't hesitate to use stuff from Guix, because it= 9;s
already a dependency, it's very close to Cuirass, and it's good to = avoid
copying code.=C2=A0 Also, don't hesitate to 'grep' for stuff li= ke
"is-directory" in the Guix repo, to find ideas.=C2=A0 It's bi= g and there are
many examples that you can use.

>=C2=A0 (define (build->hydra-build build)
>=C2=A0 =C2=A0 "Convert BUILD to an assoc list matching hydra API f= ormat."
>=C2=A0 =C2=A0 (define (bool->int bool)
> @@ -99,11 +134,12 @@ Hydra format."
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 (match key-symbol
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ('id (string->number param))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ('nr (string->number param))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 ('page (string->number param))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (_=C2=A0 =C2=A0param)))))))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(string-split qu= ery #\&))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 '())))
>=C2=A0
> -=0C
> +
=C2=A0 =C2=A0^
Here you removed the page break (^L) :-)

>=C2=A0 ;;;
>=C2=A0 ;;; Web server.
>=C2=A0 ;;;
> @@ -112,9 +148,15 @@ Hydra format."
>=C2=A0 ;;; https://github.com/NixOS= /hydra/blob/master/doc/manual/api.xml
>=C2=A0 ;;;
>=C2=A0
> +
=C2=A0 ^
Here you added a line.

>=C2=A0 (define (request-path-components request)
>=C2=A0 =C2=A0 (split-and-decode-uri-path (uri-path (request-uri request= ))))
>=C2=A0
> +(define (normalize-parameter parameter)
> + (if parameter
> +=C2=A0 =C2=A0 (list-ref parameter 0)
> +=C2=A0 =C2=A0 #f))

You don't need this function, see below.

>=C2=A0 (define (url-handler request body db-channel)
>=C2=A0
>=C2=A0 =C2=A0 (define* (respond response #:key body (db-channel db-chan= nel))
> @@ -136,6 +178,24 @@ Hydra format."
>=C2=A0 =C2=A0 =C2=A0 =C2=A0(object->json-string
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 `((error . ,message)))))

[...]

> +=C2=A0 =C2=A0 (("jobset" name)
> +=C2=A0 =C2=A0 =C2=A0(let* ((evaluation-id-max (with-critical-section = db-channel (db) (db-get-evaluations-id-max db name)))
> +=C2=A0 =C2=A0 =C2=A0 (evaluation-id-min (with-critical-section db-cha= nnel (db) (db-get-evaluations-id-min db name)))

Could you please stick to 80 columns?=C2=A0 I've noticed this at other = places
too.

> +=C2=A0 =C2=A0 =C2=A0 (params (request-parameters request))
> +=C2=A0 =C2=A0 =C2=A0 (border-high (normalize-parameter (assq-ref para= ms 'border-high)))
> +=C2=A0 =C2=A0 =C2=A0 (border-low (normalize-parameter (assq-ref param= s 'border-low))))

We need to rewrite request-parameters so that it returns an assoc-list,
but in the meantime it's better to use the 'assqx-ref' function= from
database.scm.=C2=A0 You probably need to get it out of 'db-get-builds&#= 39;
though, and to export it.

> +=C2=A0 =C2=A0 =C2=A0 (respond-html
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(html-page
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0name
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(evaluation-info-table
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 name
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (with-critical-section db-channel = (db)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (db-get-evaluations-build-s= ummary
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 db
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 name
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 PAGESIZE
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 border-low
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 border-high
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ))

Please avoid dangling parentheses :-)

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 evaluation-id-min
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 evaluation-id-max)))))
> +
> +=C2=A0 =C2=A0 (("eval" id)
> +=C2=A0 =C2=A0 =C2=A0(let* ((builds-id-max (with-critical-section db-c= hannel (db) (db-get-builds-id-max db id)))
> +=C2=A0 =C2=A0 =C2=A0 (builds-id-min (with-critical-section db-channel= (db) (db-get-builds-id-min db id)))

Here I think you can put the '(with-critical-section db-channel (db)= 9;
around the 'let*', so that you don't need to call it twice.=C2= =A0 Same for
"jobset".

> +=C2=A0 =C2=A0 =C2=A0 (params (request-parameters request))
> +=C2=A0 =C2=A0 =C2=A0 (border-high (normalize-parameter (assq-ref para= ms 'border-high)))
> +=C2=A0 =C2=A0 =C2=A0 (border-low (normalize-parameter (assq-ref param= s 'border-low))))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(respond-html
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(html-page
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "Evaluations"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (build-eval-table
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (handle-builds-request db-c= hannel
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 `((evaluation ,id)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(nr ,PAGESIZE)

See below, if it's a parameter, it would be (nr ,(%pagesize)).

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(order finish-time)=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(border-high ,borde= r-high)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(border-low ,border= -low)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 builds-id-min
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 builds-id-max)))))
> +
> +=C2=A0 =C2=A0 (("static" path ...)
> +=C2=A0 =C2=A0 =C2=A0;(display (request-uri request))
> +=C2=A0 =C2=A0 =C2=A0(respond-static-file path))
>=C2=A0 =C2=A0 =C2=A0 ('method-not-allowed
>=C2=A0 =C2=A0 =C2=A0 =C2=A0;; 405 "Method Not Allowed"
>=C2=A0 =C2=A0 =C2=A0 =C2=A0(values (build-response #:code 405) #f db-ch= annel))
>=C2=A0 =C2=A0 =C2=A0 (_
> -=C2=A0 =C2=A0 =C2=A0(respond (build-response #:code 404)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 #:body (string-appen= d "Resource not found: "
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (uri->string (r= equest-uri request)))))))
> +=C2=A0 =C2=A0 =C2=A0(respond-not-found=C2=A0 (uri->string (request= -uri request))))))
>=C2=A0
>=C2=A0 (define* (run-cuirass-server db #:key (host "localhost"= ;) (port 8080))
>=C2=A0 =C2=A0 (let* ((host-info=C2=A0 (gethostbyname host))
> diff --git a/src/cuirass/templates.scm b/src/cuirass/templates.scm
> new file mode 100644
> index 0000000..0e72981
> --- /dev/null
> +++ b/src/cuirass/templates.scm
> @@ -0,0 +1,207 @@
> +
> +;;;; http.scm -- HTTP API
> +;;; Copyright =C2=A9 2018 Tatiana Sholokhova <tanja201396@gmail.com>
> +;;;
> +;;; This file is part of Cuirass.
> +;;;
> +;;; Cuirass is free software: you can redistribute it and/or modify > +;;; it under the terms of the GNU General Public License as published= by
> +;;; the Free Software Foundation, either version 3 of the License, or=
> +;;; (at your option) any later version.
> +;;;
> +;;; Cuirass is distributed in the hope that it will be useful,
> +;;; but WITHOUT ANY WARRANTY; without even the implied warranty of > +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.=C2=A0 See th= e
> +;;; GNU General Public License for more details.
> +;;;
> +;;; You should have received a copy of the GNU General Public License=
> +;;; along with Cuirass.=C2=A0 If not, see <http://www.gnu.org/l= icenses/>.
> +
> +(define-module (cuirass templates)
> +=C2=A0 #:export (html-page
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 specifications-table
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 build-table
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 evaluation-info-table
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 build-eval-table
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 PAGESIZE))
> +
> +(define PAGESIZE 10)

It's better to use Guile parameters[1] for the reasons explained above.=
Also, our convention is to start global variables' names with a '%&= #39;.
That would give:

(define %pagesize
=C2=A0 ;; description
=C2=A0 (make-parameter 10))

[1]: https://www.gnu.org/software= /guile/manual/html_node/Parameters.html

> +(define (html-page title body)
> +=C2=A0 "Return html page with given title and body"
> +=C2=A0 `(html (@ (xmlns "http://www.w3.org/1999/xhtml&quo= t;) (xml:lang "en") (lang "en"))
> +=C2=A0 =C2=A0 (head
> +=C2=A0 =C2=A0 =C2=A0 (meta (@ (charset "utf-8")))
> +=C2=A0 =C2=A0 =C2=A0 (meta (@ (name "viewport")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(content "= ;width=3Ddevice-width, initial-scale=3D1, shrink-to-fit=3Dno")))
> +=C2=A0 =C2=A0 =C2=A0 (link (@ (rel "stylesheet")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(href "/s= tatic/css/bootstrap.css")))
> +=C2=A0 =C2=A0 =C2=A0 (link (@ (rel "stylesheet")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(href "/s= tatic/css/open-iconic-bootstrap.css")))
> +=C2=A0 =C2=A0 =C2=A0 (title ,title))
> +=C2=A0 =C2=A0 (body
> +=C2=A0 =C2=A0 =C2=A0 (nav (@ (class "navbar navbar-expand-lg nav= bar-light bg-light"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(a (@ (class "navbar-br= and") (href "/"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (img (@ (src "/= static/images/logo.png")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 (alt "logo")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 (height "25")))))
> +=C2=A0 =C2=A0 =C2=A0 (main (@ (role "main") (class "co= ntainer pt-4 px-1"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ,body
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (hr)))))
> +
> +
> +(define (specifications-table specs)
> +=C2=A0 "Return body for main (Projects) html-page"

I think it's good to refer to the arguments while describing the
function.=C2=A0 I'd use: "Return HTML for the SPECS table." o= r something
similar.=C2=A0 I think this function could be used in another context than = a
"body" (please correct me if I'm wrong), so there is no need = to put
"body" in the docstring.

> +=C2=A0 `((p (@ (class "lead")) "Projects")
> +=C2=A0 =C2=A0 (table
> +=C2=A0 =C2=A0 =C2=A0 (@ (class "table table-sm table-hover"= ))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0,@(if (null? specs)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 `((th (@ (scope "col")) = "No elements here."))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 `((thead
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (tr
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope = "col")) Name)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope = "col")) Branch)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(tbody
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ,@(map
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(lambda (spec)=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 `(tr
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (td (a= (@ (href "/jobset/" ,(assq-ref spec #:name))) ,(assq-ref spec #:= name)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (td ,(= assq-ref spec #:branch))))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 specs)))))))
> +
> +(define (pagination page-id-min page-id-max id-min id-max)
> +=C2=A0 "Return page navigation buttons"
> +=C2=A0 =C2=A0 `(div (@ (class row))
> +=C2=A0 =C2=A0 =C2=A0 (nav
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 (@ (class "mx-auto") (aria-labe= l "Page navigation"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 (ul (@ (class "pagination")) > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (li (@ (class "page-it= em"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (a (@ (class = "page-link")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (href "?border-high=3D" ,(number->string (+ id-max 1))))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= "<< First"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (li (@ (class "page-it= em" ,(if (=3D page-id-max id-max) " disabled" ""))= )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (a (@ (class = "page-link")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (href "?border-low=3D" ,(number->string page-id-max)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= "< Previous"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (li (@ (class "page-it= em" ,(if (=3D page-id-min id-min) " disabled" ""))= )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (a (@ (class = "page-link")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (href "?border-high=3D" ,(number->string page-id-min)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= "Next >"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (li (@ (class "page-it= em"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (a (@ (class = "page-link")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (href "?border-low=3D" ,(number->string (- id-min 1))))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= "Last >>"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ))))

Could you give a status about the pagination?=C2=A0 I have seen several
discussions about it.=C2=A0 The goal for this commit is to have something that we can use, but it doesn't need to be perfect.=C2=A0 We can improv= e it
later.

> +(define (minimum lst cur-min)

I have no idea what cur-min means :-)=C2=A0 Could you find a more descripti= ve
name?

> +=C2=A0 (cond ((null? lst) cur-min)
> +=C2=A0 =C2=A0 =C2=A0 ((< (car lst) cur-min) (minimum (cdr lst) (ca= r lst)))
> +=C2=A0 =C2=A0 =C2=A0 (else (minimum (cdr lst) cur-min))))
> +
=C2=A0^ extra line :-)
> +
> +(define (maximum lst cur-max)
> +=C2=A0 (cond ((null? lst) cur-max)
> +=C2=A0 =C2=A0 =C2=A0 ((> (car lst) cur-max) (maximum (cdr lst) (ca= r lst)))
> +=C2=A0 =C2=A0 =C2=A0 (else (maximum (cdr lst) cur-max))))
> +
=C2=A0^ too
> +
> +(define (evaluation-info-table name data evaluation-id-min evaluation= -id-max)
> +=C2=A0 "Return body for (Evaluation) html-page"

"Return HTML for the EVALUATION table NAME from EVALUATION-ID-MIN to EVALUATION-ID-MAX." (and please, replace 'data' with 'eval= uations', it's
difficult to know what 'data' means :-)).

> +=C2=A0 (let ((id-min (minimum (map (lambda (row) (assq-ref row #:id))= data) evaluation-id-max))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(id-max (maximum (map (lambda (row) (assq-= ref row #:id)) data) evaluation-id-min)))
> +=C2=A0 =C2=A0 `((p (@ (class "lead")) "Evaluations of = " ,name)
> +=C2=A0 =C2=A0 =C2=A0 ;(p (@ (class "text-muted")) "Sho= wing evaluations ",id-min "-",id-max " out of ",ev= aluation-id-max)
> +=C2=A0 =C2=A0 =C2=A0 (table
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 (@ (class "table table-sm table-hove= r table-striped"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0,@(if (null? data)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 `((th (@ (scope "col&q= uot;)) "No elements here."))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 `((thead
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (tr
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ = (scope "col")) "#")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ = (scope "col")) Revision)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ = (scope "col")) Success)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(tbody
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ,@(map
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(lambda= (row)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 `(tr > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (th (@ (scope "row")) (a (@ (href "/eval/" ,(assq-ref = row #:id))) ,(assq-ref row #:id)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (td ,(assq-ref row #:revision))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (td
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 (a (@ (href "#") (class "badge badge-success"))= ,(assq-ref row #:succeeded))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 (a (@ (href "#") (class "badge badge-danger")) = ,(assq-ref row #:failed))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 (a (@ (href "#") (class "badge badge-secondary"= )) ,(assq-ref row #:scheduled)))))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0data)))= ))
> +=C2=A0 =C2=A0 =C2=A0 ,(pagination id-min id-max evaluation-id-min eva= luation-id-max))))
> +
> +(define (build-eval-table data build-id-min build-id-max)
> +
=C2=A0^ extra line
> +=C2=A0 (define (table-header)
> +=C2=A0 =C2=A0 `(thead
> +=C2=A0 =C2=A0 =C2=A0 (tr
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) '()) > +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) ID)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) Project) > +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) "Fini= shed at")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) Job)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) Nixname) > +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) System)
[...]

> +(define (build-table done pending)
> +=C2=A0 "Return body for project's html-page"
> +=C2=A0 (define (table-row build)
> +=C2=A0 =C2=A0 `(tr
> +=C2=A0 =C2=A0 =C2=A0 (td ,(assq-ref build #:project))
> +=C2=A0 =C2=A0 =C2=A0 (td ,(assq-ref build #:jobset))
> +=C2=A0 =C2=A0 =C2=A0 (td ,(assq-ref build #:job))
> +=C2=A0 =C2=A0 =C2=A0 (td ,(assq-ref build #:nixname))
> +=C2=A0 =C2=A0 =C2=A0 (td ,(assq-ref build #:buildstatus))))
> +=C2=A0 (define (table-header)
> +=C2=A0 =C2=A0 `(thead
> +=C2=A0 =C2=A0 =C2=A0 (tr
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) Project) > +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) Jobset) > +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) Job)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) Nixname) > +=C2=A0 =C2=A0 =C2=A0 =C2=A0(th (@ (scope "col")) Buildstatu= s))))
> +=C2=A0 `((table
> +=C2=A0 =C2=A0 =C2=A0(@ (class "table table-sm table-hover table-= striped"))
> +=C2=A0 =C2=A0 =C2=A0(caption=C2=A0 "Latest builds")
> +=C2=A0 =C2=A0 =C2=A0,@(if (null? done)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0`((th (@ (scope "col")) "No= elements here."))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0`(,(table-header)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(tbody
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (@ (class "table table-sm tab= le-hover table-striped"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ,@(map table-row done)))))
> +=C2=A0 =C2=A0 (table
> +=C2=A0 =C2=A0 =C2=A0(@ (class "table table-sm table-hover table-= striped"))
> +=C2=A0 =C2=A0 =C2=A0(caption "Queue")
> +=C2=A0 =C2=A0 =C2=A0,@(if (null? pending)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0`((th (@ (scope "col")) "No= elements here."))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0`(,(table-header)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(tbody
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (@ (class "table table-sm tab= le-hover table-striped"))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ,@(map table-row pending)))))))
Here I would do a procedure 'build-table' of one argument, that I w= ould
call twice: once with the builds that are done, and once with the builds that are pending.=C2=A0 That would avoid the code repetition.

Also, if you need help with git (to put the commits together), don't hesistate to ask, either here or on IRC (#guix).

And please, make sure 'make check' works :-).=C2=A0 I had to replac= e (3
"/baz.drv") with (1 "/foo.drv") in the "db-get-bui= lds" test, but I
didn't look closely at it.

And that's all!


Thanks for all of you! Great work.
As far as I can see most of the suggestions are on style. Do you thi= nk that
we need some improvement in the documentation regarding s= tyle, both coding and
documentation?

=C2= =A0
Thank you!
Cl=C3=A9ment

--000000000000fc2faa05716a8358--