* Web interface review
@ 2018-07-19 21:23 Clément Lassieur
2018-07-20 8:59 ` Gábor Boskovits
2018-07-27 20:44 ` Clément Lassieur
0 siblings, 2 replies; 8+ messages in thread
From: Clément Lassieur @ 2018-07-19 21:23 UTC (permalink / raw)
To: Tatiana Sholokhova, guix-devel
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
:-).
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 = $(guileobjectdir)/$(PACKAGE)
webmoduledir = $(guilesitedir)/web/server
webobjectdir = $(guileobjectdir)/web/server
sqldir = $(pkgdatadir)/sql
+staticdir = $(pkgdatadir)/static
+cssdir = $(staticdir)/css
+fontsdir = $(staticdir)/fonts
+imagesdir = $(staticdir)/images
dist_pkgmodule_DATA = \
src/cuirass/base.scm \
@@ -62,6 +66,18 @@ dist_pkgdata_DATA = src/schema.sql
dist_sql_DATA = \
src/sql/upgrade-1.sql
+dist_css_DATA = \
+ src/static/css/bootstrap.css \
+ src/static/css/open-iconic-bootstrap.css
+dist_fonts_DATA = \
+ 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 = \
+ src/static/images/logo.png
+
TEST_EXTENSIONS = .scm .sh
AM_TESTS_ENVIRONMENT = \
env GUILE_AUTO_COMPILE='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=0) 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=" 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=E.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=" 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=" 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=" 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=" 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=" 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 © 2016 Mathieu Lirzin <mthl@gnu.org>
> ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
> ;;; Copyright © 2018 Ludovic Courtès <ludo@gnu.org>
> ;;; Copyright © 2018 Clément Lassieur <clement@lassieur.org>
> +;;; Copyright © 2018 Tatiana Sholokhova <tanja201396@gmail.com>
> ;;;
> ;;; 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=? 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 #\&))
> '())))
>
> -\f
> +
^
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 © 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. 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 <http://www.gnu.org/licenses/>.
> +
> +(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=device-width, initial-scale=1, shrink-to-fit=no")))
> + (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=" ,(number->string (+ id-max 1))))
> + "<< First"))
> + (li (@ (class "page-item" ,(if (= page-id-max id-max) " disabled" "")))
> + (a (@ (class "page-link")
> + (href "?border-low=" ,(number->string page-id-max)))
> + "< Previous"))
> + (li (@ (class "page-item" ,(if (= page-id-min id-min) " disabled" "")))
> + (a (@ (class "page-link")
> + (href "?border-high=" ,(number->string page-id-min)))
> + "Next >"))
> + (li (@ (class "page-item"))
> + (a (@ (class "page-link")
> + (href "?border-low=" ,(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!
Thank you!
Clément
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Web interface review
2018-07-19 21:23 Web interface review Clément Lassieur
@ 2018-07-20 8:59 ` Gábor Boskovits
2018-07-20 9:28 ` Clément Lassieur
2018-07-27 20:44 ` Clément Lassieur
1 sibling, 1 reply; 8+ messages in thread
From: Gábor Boskovits @ 2018-07-20 8:59 UTC (permalink / raw)
To: Clément Lassieur; +Cc: Guix-devel, Tatiana Sholokhova
[-- Attachment #1: Type: text/plain, Size: 26456 bytes --]
Clément Lassieur <clement@lassieur.org> ezt írta (időpont: 2018. júl. 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 = $(guileobjectdir)/$(PACKAGE)
> webmoduledir = $(guilesitedir)/web/server
> webobjectdir = $(guileobjectdir)/web/server
> sqldir = $(pkgdatadir)/sql
> +staticdir = $(pkgdatadir)/static
> +cssdir = $(staticdir)/css
> +fontsdir = $(staticdir)/fonts
> +imagesdir = $(staticdir)/images
>
> dist_pkgmodule_DATA = \
> src/cuirass/base.scm \
> @@ -62,6 +66,18 @@ dist_pkgdata_DATA = src/schema.sql
> dist_sql_DATA = \
> src/sql/upgrade-1.sql
>
> +dist_css_DATA = \
> + src/static/css/bootstrap.css \
> + src/static/css/open-iconic-bootstrap.css
> +dist_fonts_DATA = \
> + 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 = \
> + src/static/images/logo.png
> +
> TEST_EXTENSIONS = .scm .sh
> AM_TESTS_ENVIRONMENT = \
> env GUILE_AUTO_COMPILE='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=0) 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=" 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=E.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=" 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=" 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=" 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=" 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=" 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 © 2016 Mathieu Lirzin <mthl@gnu.org>
> > ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
> > ;;; Copyright © 2018 Ludovic Courtès <ludo@gnu.org>
> > ;;; Copyright © 2018 Clément Lassieur <clement@lassieur.org>
> > +;;; Copyright © 2018 Tatiana Sholokhova <tanja201396@gmail.com>
> > ;;;
> > ;;; 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=? 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 © 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. 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 <http://www.gnu.org/licenses/>.
> > +
> > +(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=device-width, initial-scale=1,
> shrink-to-fit=no")))
> > + (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=" ,(number->string (+ id-max
> 1))))
> > + "<< First"))
> > + (li (@ (class "page-item" ,(if (= page-id-max id-max) "
> disabled" "")))
> > + (a (@ (class "page-link")
> > + (href "?border-low=" ,(number->string page-id-max)))
> > + "< Previous"))
> > + (li (@ (class "page-item" ,(if (= page-id-min id-min) "
> disabled" "")))
> > + (a (@ (class "page-link")
> > + (href "?border-high=" ,(number->string page-id-min)))
> > + "Next >"))
> > + (li (@ (class "page-item"))
> > + (a (@ (class "page-link")
> > + (href "?border-low=" ,(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ément
>
>
[-- Attachment #2: Type: text/html, Size: 34621 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Web interface review
2018-07-20 8:59 ` Gábor Boskovits
@ 2018-07-20 9:28 ` Clément Lassieur
2018-07-21 14:08 ` Tatiana Sholokhova
0 siblings, 1 reply; 8+ messages in thread
From: Clément Lassieur @ 2018-07-20 9:28 UTC (permalink / raw)
To: Gábor Boskovits; +Cc: Guix-devel, Tatiana Sholokhova
Gábor Boskovits <boskovits@gmail.com> writes:
> 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?
I don't know, but I think encouraging people to copy the existing style
is a good starting point. :-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Web interface review
2018-07-20 9:28 ` Clément Lassieur
@ 2018-07-21 14:08 ` Tatiana Sholokhova
2018-07-21 14:50 ` Clément Lassieur
0 siblings, 1 reply; 8+ messages in thread
From: Tatiana Sholokhova @ 2018-07-21 14:08 UTC (permalink / raw)
To: Clément Lassieur; +Cc: guix-devel
[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]
Hello Clément!
Thank you for your review!
I fixed most of the problems you noticed and rebased commits as you advised
.
I couldn't fix the problem with several calling of (with-critical-section).
As I wrote to IRC channel, I tried to put '(with-critical-section
db-channel (db)' around '(let* ...)' and I received an error:
```
In web/server.scm:
279:25 0 (_)
Throw to key `vm-error' with args `(vm-run "Too few values returned to
continuation" ())'.
```
Could you give a status about the pagination?
>
Pagination works correctly with evaluations, but it doesn't work correctly
with builds. In some cases, we have builds missing. It happens due to equal
timestamp values, so we need to filter build by (timestamp, id) tuple key.
What else do we need to do before the merge?
Best regards,
Tatiana
пт, 20 июл. 2018 г. в 11:28, Clément Lassieur <clement@lassieur.org>:
> Gábor Boskovits <boskovits@gmail.com> writes:
>
> > 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?
>
> I don't know, but I think encouraging people to copy the existing style
> is a good starting point. :-)
>
[-- Attachment #2: Type: text/html, Size: 2460 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Web interface review
2018-07-21 14:08 ` Tatiana Sholokhova
@ 2018-07-21 14:50 ` Clément Lassieur
2018-07-22 14:57 ` Tatiana Sholokhova
0 siblings, 1 reply; 8+ messages in thread
From: Clément Lassieur @ 2018-07-21 14:50 UTC (permalink / raw)
To: Tatiana Sholokhova; +Cc: guix-devel
Hello Tatiana!
Tatiana Sholokhova <tanja201396@gmail.com> writes:
> Hello Clément!
>
> Thank you for your review!
>
> I fixed most of the problems you noticed and rebased commits as you advised
> .
>
> I couldn't fix the problem with several calling of (with-critical-section).
> As I wrote to IRC channel, I tried to put '(with-critical-section
> db-channel (db)' around '(let* ...)' and I received an error:
> ```
> In web/server.scm:
> 279:25 0 (_)
> Throw to key `vm-error' with args `(vm-run "Too few values returned to
> continuation" ())'.
> ```
It's because 'respond-html' returns several values. I think you could
do:
(respond-html
(with-critical-section ...
(let* ...)))
> Could you give a status about the pagination?
>>
> Pagination works correctly with evaluations, but it doesn't work correctly
> with builds. In some cases, we have builds missing. It happens due to equal
> timestamp values, so we need to filter build by (timestamp, id) tuple key.
>
> What else do we need to do before the merge?
Once we have something consistent, we can push. And we can add stuff
afterwards of course. Do you think it would be feasible to fix the
pagination before the merge?
I won't have time to look at your update before tomorrow night, I'll let
you know then!
Thanks,
Clément
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Web interface review
2018-07-21 14:50 ` Clément Lassieur
@ 2018-07-22 14:57 ` Tatiana Sholokhova
2018-07-22 22:56 ` Tatiana Sholokhova
0 siblings, 1 reply; 8+ messages in thread
From: Tatiana Sholokhova @ 2018-07-22 14:57 UTC (permalink / raw)
To: Clément Lassieur; +Cc: guix-devel
[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]
Hi Clement,
Thank you for your advice! I resolved the 'with-critical-section' issue. To
do so, I had to remove 'with-critical-section' call from
'handle-build-request' function and wrap critical section around each call
of this function.
Today I am trying to fix the pagination and I will let you know about my
results.
P. S.
Now I send this changes as separate small commits, we can rebase the
history again when I finish with pagination.
Best regards,
Tatiana
сб, 21 июл. 2018 г. в 16:50, Clément Lassieur <clement@lassieur.org>:
> Hello Tatiana!
>
> Tatiana Sholokhova <tanja201396@gmail.com> writes:
>
> > Hello Clément!
> >
> > Thank you for your review!
> >
> > I fixed most of the problems you noticed and rebased commits as you
> advised
> > .
> >
> > I couldn't fix the problem with several calling of
> (with-critical-section).
> > As I wrote to IRC channel, I tried to put '(with-critical-section
> > db-channel (db)' around '(let* ...)' and I received an error:
> > ```
> > In web/server.scm:
> > 279:25 0 (_)
> > Throw to key `vm-error' with args `(vm-run "Too few values returned to
> > continuation" ())'.
> > ```
>
> It's because 'respond-html' returns several values. I think you could
> do:
>
> (respond-html
> (with-critical-section ...
> (let* ...)))
>
> > Could you give a status about the pagination?
> >>
> > Pagination works correctly with evaluations, but it doesn't work
> correctly
> > with builds. In some cases, we have builds missing. It happens due to
> equal
> > timestamp values, so we need to filter build by (timestamp, id) tuple
> key.
> >
> > What else do we need to do before the merge?
>
> Once we have something consistent, we can push. And we can add stuff
> afterwards of course. Do you think it would be feasible to fix the
> pagination before the merge?
>
> I won't have time to look at your update before tomorrow night, I'll let
> you know then!
>
> Thanks,
> Clément
>
[-- Attachment #2: Type: text/html, Size: 2843 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Web interface review
2018-07-22 14:57 ` Tatiana Sholokhova
@ 2018-07-22 22:56 ` Tatiana Sholokhova
0 siblings, 0 replies; 8+ messages in thread
From: Tatiana Sholokhova @ 2018-07-22 22:56 UTC (permalink / raw)
To: Clément Lassieur; +Cc: guix-devel
[-- Attachment #1: Type: text/plain, Size: 2825 bytes --]
Hi again!
I have implemented the pagination and it works correctly now. It took me
some time and eventually I have figured out that we don't need any special
procedure to find minimal/maximum (timestamp, id) pair for builds in guile
code since we already receive builds in the proper ordering from the
database. So, we can extract first and last of the current page entries and
that's it. :)
Besides that, I fixed some of the issues covered in the last review which I
missed before. I am looking forward to merging the branches. Let me know if
there is still something that needs to be fixed.
Best regards,
Tatiana
вс, 22 июл. 2018 г. в 16:57, Tatiana Sholokhova <tanja201396@gmail.com>:
> Hi Clement,
>
> Thank you for your advice! I resolved the 'with-critical-section' issue.
> To do so, I had to remove 'with-critical-section' call from
> 'handle-build-request' function and wrap critical section around each call
> of this function.
>
> Today I am trying to fix the pagination and I will let you know about my
> results.
>
> P. S.
> Now I send this changes as separate small commits, we can rebase the
> history again when I finish with pagination.
>
> Best regards,
> Tatiana
>
>
> сб, 21 июл. 2018 г. в 16:50, Clément Lassieur <clement@lassieur.org>:
>
>> Hello Tatiana!
>>
>> Tatiana Sholokhova <tanja201396@gmail.com> writes:
>>
>> > Hello Clément!
>> >
>> > Thank you for your review!
>> >
>> > I fixed most of the problems you noticed and rebased commits as you
>> advised
>> > .
>> >
>> > I couldn't fix the problem with several calling of
>> (with-critical-section).
>> > As I wrote to IRC channel, I tried to put '(with-critical-section
>> > db-channel (db)' around '(let* ...)' and I received an error:
>> > ```
>> > In web/server.scm:
>> > 279:25 0 (_)
>> > Throw to key `vm-error' with args `(vm-run "Too few values returned to
>> > continuation" ())'.
>> > ```
>>
>> It's because 'respond-html' returns several values. I think you could
>> do:
>>
>> (respond-html
>> (with-critical-section ...
>> (let* ...)))
>>
>> > Could you give a status about the pagination?
>> >>
>> > Pagination works correctly with evaluations, but it doesn't work
>> correctly
>> > with builds. In some cases, we have builds missing. It happens due to
>> equal
>> > timestamp values, so we need to filter build by (timestamp, id) tuple
>> key.
>> >
>> > What else do we need to do before the merge?
>>
>> Once we have something consistent, we can push. And we can add stuff
>> afterwards of course. Do you think it would be feasible to fix the
>> pagination before the merge?
>>
>> I won't have time to look at your update before tomorrow night, I'll let
>> you know then!
>>
>> Thanks,
>> Clément
>>
>
[-- Attachment #2: Type: text/html, Size: 3910 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Web interface review
2018-07-19 21:23 Web interface review Clément Lassieur
2018-07-20 8:59 ` Gábor Boskovits
@ 2018-07-27 20:44 ` Clément Lassieur
1 sibling, 0 replies; 8+ messages in thread
From: Clément Lassieur @ 2018-07-27 20:44 UTC (permalink / raw)
To: Tatiana Sholokhova, guix-devel
Clément Lassieur <clement@lassieur.org> writes:
>> +"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.
For the record, I think I was wrong here, as the SQLite documentation[1]
says:
Ordinary SQL statements are free-form, and can be spread across
multiple lines, and can have whitespace and comments anywhere.
I've no idea why this '\' style was adopted, I think I'll stop using it.
Clément
[1]: https://www.sqlite.org/cli.html#rules_for_dot_commands_
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-27 20:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-19 21:23 Web interface review Clément Lassieur
2018-07-20 8:59 ` Gábor Boskovits
2018-07-20 9:28 ` Clément Lassieur
2018-07-21 14:08 ` Tatiana Sholokhova
2018-07-21 14:50 ` Clément Lassieur
2018-07-22 14:57 ` Tatiana Sholokhova
2018-07-22 22:56 ` Tatiana Sholokhova
2018-07-27 20:44 ` Clément Lassieur
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.