I read some of the code, I like it :)
diff --git a/src/cuirass/base.scm b/src/cuirass/base.scm
index 82f49a4..af24a28 100644
--- a/src/cuirass/base.scm
+++ b/src/cuirass/base.scm
@@ -20,6 +20,10 @@
;;; You should have received a copy of the GNU General Public License
;;; along with Cuirass. If not, see <http://www.gnu.org/licenses/>.
+;; Comment:
+
+;; This is called base because it interface with guix daemon.
+
(define-module (cuirass base)
#:use-module (fibers)
#:use-module (cuirass logging)
diff --git a/src/cuirass/http.scm b/src/cuirass/http.scm
index 2d66ff9..0899cd1 100644
--- a/src/cuirass/http.scm
+++ b/src/cuirass/http.scm
@@ -117,6 +117,13 @@
(map build->hydra-build builds)))
(define (request-parameters request)
+ ;; REVIEW: this 'parameters' is ambigious in aiohttp. It's called
+ ;; request.query because the part after ? in the URL is called the "query
+ ;; string". Also parameters can be passed via the path part of the URL.
+
+ ;; here is what I use
+ ;;
https://github.com/a-guile-mind/guile-web/blob/master/src/web/decode.scm
+
"Parse the REQUEST query parameters and return them under the form
'((parameter . value) ...)."
(let* ((uri (request-uri request))
@@ -125,11 +132,14 @@
(map (lambda (param)
(match (string-split param #\=)
((key param)
- (let ((key-symbol (string->symbol key)))
+ (let ((key-symbol (string->symbol (uri-decode key))))
(cons key-symbol
(match key-symbol
- ('id (string->number param))
- ('nr (string->number param))
+ ('id (string->number (uri-decode param)))
+ ;; I don't think this will scale in terms
of
+ ;; kind query pairs where the key part
can be
+ ;; many different things
+ ('nr (string->number (uri-decode param)))
(_ param)))))))
(string-split query #\&))
'())))
@@ -138,9 +148,10 @@
;;;
;;; Web server.
;;;
-;;; The api is derived from the hydra one. It is partially described here :
+;;; The exposed api is derived from the hydra one. It is partially
described
+;;; here :
;;;
-;;; https://github.com/NixOS/hydra/blob/master/doc/manual/api.xml
+;;; https://github.com/NixOS/hydra/blob/master/doc/manual/api.xml
;;;
(define (request-path-components request)
@@ -217,6 +228,8 @@
(with-critical-section db-channel (db)
(db-get-specifications db)))))
(("build" build-id)
+ ;; REVIEW: don't inline that much code in same procedure aka.
procedures
+ ;; for the win1
(let ((hydra-build
(with-critical-section db-channel (db)
(handle-build-request db (string->number build-id)))))
@@ -234,6 +247,7 @@
(let ((uri (string->uri-reference
(string-append "/log/"
(basename output)))))
+ ;; create a procedure for that when you will have forms.
(respond (build-response #:code 302
#:headers `((location . ,uri)))
#:body "")))
@@ -241,7 +255,10 @@
;; Not entry for BUILD-ID in the 'Outputs' table.
(respond-json-with-error
500
+ ;; 500 is very strong error, it's must not be used for
expected failure
+ ;; by the way this is rendering logic, it should be in it's
own procedure.
(format #f "Outputs of build ~a are unknown." build-id)))
+
(#f
(respond-build-not-found build-id)))
(respond-build-not-found build-id))))
@@ -251,13 +268,25 @@
(nr (assq-ref params 'nr)))
(if nr
(respond-json (object->json-string
+ ;; don't nest here the code. Because http
handlers should follow
+ ;; the following pattern:
+ ;;
+ ;; (define (handler request body)
+ ;; (let ((input (snarf-input request body)))
+ ;; (shallow-validate-with-exception
input)
+ ;; (deep-validate-with-exception input
(db))
+ ;; (let ((out (domain-code-goes-here)))
+ ;; (sxml->response (handler-template
out))))
+ ;;
+ ;; Basically, the current code requires top down
and bottom up
+ ;; reading. Declaring intermediate variables
helps readbility.
(with-critical-section db-channel (db)
(db-get-evaluations db nr))))
- (respond-json-with-error 500 "Parameter not defined!"))))
+ (respond-json-with-error 400 "Parameter not defined!")))) ;;
aka. bad request
(("api" "latestbuilds")
(let* ((params (request-parameters request))
;; 'nr parameter is mandatory to limit query size.
- (valid-params? (assq-ref params 'nr)))
+ (valid-params? (assq-ref params 'nr))) ;; or exception?
(if valid-params?
;; Limit results to builds that are "done".
(respond-json
@@ -359,7 +388,10 @@
;; process each client request and then directly go back waiting for
the
;; next client (conversely, Guile's 'run-server' loop processes clients
;; one after another, sequentially.) We can do that because we don't
- ;; maintain any state across connections.
+ ;; maintain any state across connections. I don't understand that
+ ;; comment. Why no state across connections? isn't sqlite that stores
+ ;; states? Does it mean there is no global mutable in memory data in
+ ;; cuirass?
;;
;; XXX: We don't do 'call-with-sigint' like 'run-server' does.
(let* ((impl (lookup-server-impl 'fiberized))
diff --git a/src/schema.sql b/src/schema.sql
index eb0f7e9..769360f 100644
--- a/src/schema.sql
+++ b/src/schema.sql
@@ -1,5 +1,8 @@
BEGIN TRANSACTION;
+-- REVIEW: The name of the table must be all small caps.
+-- COMMENT: Some people argue table name must be singular
+-- but I disagree, both might work.
CREATE TABLE Specifications (
name TEXT NOT NULL PRIMARY KEY,
load_path_inputs TEXT NOT NULL, -- list of input names whose load path
will be in Guile's %load-path
@@ -64,7 +67,7 @@ CREATE TABLE Builds (
log TEXT NOT NULL,
status INTEGER NOT NULL,
timestamp INTEGER NOT NULL,
- starttime INTEGER NOT NULL,
+ starttime INTEGER NOT NULL, -- REVIEW: why not start_time?
stoptime INTEGER NOT NULL,
FOREIGN KEY (derivation) REFERENCES Derivations (derivation),
FOREIGN KEY (evaluation) REFERENCES Evaluations (id)
@@ -75,5 +78,12 @@ CREATE TABLE Builds (
CREATE INDEX Builds_Derivations_index ON Builds(status ASC, timestamp ASC,
id, derivation, evaluation, stoptime DESC);
CREATE INDEX Inputs_index ON Inputs(specification, name, branch);
CREATE INDEX Derivations_index ON Derivations(derivation, evaluation,
job_name, system);
+-- COMMENT: Indices make the following trade off: slow down writes and take
+-- more space (disk and memory) and in prize you get faster reads.
+-- COMMENT: you'd better add CREATE INDEX just below the table that they
+-- speed up queries for.
+
COMMIT;
+
+--
Le mer. 1 août 2018 à 21:47, Tatiana Sholokhova <tanja201...@gmail.com> a
écrit :
> Hi all,
>
> Thank you for the congratulations! I am excited to have the web-interface
> pushed into the master branch. I would like to thank you all for your great
> support. Special thanks to Clément for helpful final reviews and merging
> all the changes together.
>
> Now I would like to add some more features to the interface to enhance it
> further. Am I right that now I should again organize my changes as a new
> separate branch?
>
> Best regards,
> Tatiana
>
> On Mon, 30 Jul 2018 at 15:27, Ricardo Wurmus <rek...@elephly.net> wrote:
>
>>
>> Hi Pjotr,
>>
>> > One comment: I think we should strive to design GSoC programmes in a
>> > way that students get to push earlier to trunk. I know Guix can be
>> > complex, but even so, I think we would have less failure if we make it
>> > small pieces and better gratification.
>>
>> This has always been our goal. The projects can usually be implemented
>> in independent chunks that could be merged into the “master” branch at
>> various stages.
>>
>> --
>> Ricardo
>>
>>
diff --git a/src/cuirass/base.scm b/src/cuirass/base.scm
index 82f49a4..af24a28 100644
--- a/src/cuirass/base.scm
+++ b/src/cuirass/base.scm
@@ -20,6 +20,10 @@
;;; You should have received a copy of the GNU General Public License
;;; along with Cuirass. If not, see <http://www.gnu.org/licenses/>.
+;; Comment:
+
+;; This is called base because it interface with guix daemon.
+
(define-module (cuirass base)
#:use-module (fibers)
#:use-module (cuirass logging)
diff --git a/src/cuirass/database.scm b/src/cuirass/database.scm
index 56f421d..ccb8309 100644
--- a/src/cuirass/database.scm
+++ b/src/cuirass/database.scm
@@ -547,6 +547,7 @@ Assumes that if group id stays the same the group headers stay the same."
;; before those in 'scheduled' state (-2).
"status DESC, timestamp DESC")
(_ "id DESC")))
+ ;; TODO: create macro to read the following SQL query from something-get-builds.sql
(stmt-text (format #f "SELECT * FROM (
SELECT Builds.id, Outputs.name, Outputs.path, Builds.timestamp,
Builds.starttime, Builds.stoptime, Builds.log, Builds.status,
diff --git a/src/cuirass/http.scm b/src/cuirass/http.scm
index 2d66ff9..0899cd1 100644
--- a/src/cuirass/http.scm
+++ b/src/cuirass/http.scm
@@ -117,6 +117,13 @@
(map build->hydra-build builds)))
(define (request-parameters request)
+ ;; REVIEW: this 'parameters' is ambigious in aiohttp. It's called
+ ;; request.query because the part after ? in the URL is called the "query
+ ;; string". Also parameters can be passed via the path part of the URL.
+
+ ;; here is what I use
+ ;; https://github.com/a-guile-mind/guile-web/blob/master/src/web/decode.scm
+
"Parse the REQUEST query parameters and return them under the form
'((parameter . value) ...)."
(let* ((uri (request-uri request))
@@ -125,11 +132,14 @@
(map (lambda (param)
(match (string-split param #\=)
((key param)
- (let ((key-symbol (string->symbol key)))
+ (let ((key-symbol (string->symbol (uri-decode key))))
(cons key-symbol
(match key-symbol
- ('id (string->number param))
- ('nr (string->number param))
+ ('id (string->number (uri-decode param)))
+ ;; I don't think this will scale in terms of
+ ;; kind query pairs where the key part can be
+ ;; many different things
+ ('nr (string->number (uri-decode param)))
(_ param)))))))
(string-split query #\&))
'())))
@@ -138,9 +148,10 @@
;;;
;;; Web server.
;;;
-;;; The api is derived from the hydra one. It is partially described here :
+;;; The exposed api is derived from the hydra one. It is partially described
+;;; here :
;;;
-;;; https://github.com/NixOS/hydra/blob/master/doc/manual/api.xml
+;;; https://github.com/NixOS/hydra/blob/master/doc/manual/api.xml
;;;
(define (request-path-components request)
@@ -217,6 +228,8 @@
(with-critical-section db-channel (db)
(db-get-specifications db)))))
(("build" build-id)
+ ;; REVIEW: don't inline that much code in same procedure aka. procedures
+ ;; for the win1
(let ((hydra-build
(with-critical-section db-channel (db)
(handle-build-request db (string->number build-id)))))
@@ -234,6 +247,7 @@
(let ((uri (string->uri-reference
(string-append "/log/"
(basename output)))))
+ ;; create a procedure for that when you will have forms.
(respond (build-response #:code 302
#:headers `((location . ,uri)))
#:body "")))
@@ -241,7 +255,10 @@
;; Not entry for BUILD-ID in the 'Outputs' table.
(respond-json-with-error
500
+ ;; 500 is very strong error, it's must not be used for expected failure
+ ;; by the way this is rendering logic, it should be in it's own procedure.
(format #f "Outputs of build ~a are unknown." build-id)))
+
(#f
(respond-build-not-found build-id)))
(respond-build-not-found build-id))))
@@ -251,13 +268,25 @@
(nr (assq-ref params 'nr)))
(if nr
(respond-json (object->json-string
+ ;; don't nest here the code. Because http handlers should follow
+ ;; the following pattern:
+ ;;
+ ;; (define (handler request body)
+ ;; (let ((input (snarf-input request body)))
+ ;; (shallow-validate-with-exception input)
+ ;; (deep-validate-with-exception input (db))
+ ;; (let ((out (domain-code-goes-here)))
+ ;; (sxml->response (handler-template out))))
+ ;;
+ ;; Basically, the current code requires top down and bottom up
+ ;; reading. Declaring intermediate variables helps readbility.
(with-critical-section db-channel (db)
(db-get-evaluations db nr))))
- (respond-json-with-error 500 "Parameter not defined!"))))
+ (respond-json-with-error 400 "Parameter not defined!")))) ;; aka. bad request
(("api" "latestbuilds")
(let* ((params (request-parameters request))
;; 'nr parameter is mandatory to limit query size.
- (valid-params? (assq-ref params 'nr)))
+ (valid-params? (assq-ref params 'nr))) ;; or exception?
(if valid-params?
;; Limit results to builds that are "done".
(respond-json
@@ -359,7 +388,10 @@
;; process each client request and then directly go back waiting for the
;; next client (conversely, Guile's 'run-server' loop processes clients
;; one after another, sequentially.) We can do that because we don't
- ;; maintain any state across connections.
+ ;; maintain any state across connections. I don't understand that
+ ;; comment. Why no state across connections? isn't sqlite that stores
+ ;; states? Does it mean there is no global mutable in memory data in
+ ;; cuirass?
;;
;; XXX: We don't do 'call-with-sigint' like 'run-server' does.
(let* ((impl (lookup-server-impl 'fiberized))
diff --git a/src/schema.sql b/src/schema.sql
index eb0f7e9..769360f 100644
--- a/src/schema.sql
+++ b/src/schema.sql
@@ -1,5 +1,8 @@
BEGIN TRANSACTION;
+-- REVIEW: The name of the table must be all small caps.
+-- COMMENT: Some people argue table name must be singular
+-- but I disagree, both might work.
CREATE TABLE Specifications (
name TEXT NOT NULL PRIMARY KEY,
load_path_inputs TEXT NOT NULL, -- list of input names whose load path will be in Guile's %load-path
@@ -64,7 +67,7 @@ CREATE TABLE Builds (
log TEXT NOT NULL,
status INTEGER NOT NULL,
timestamp INTEGER NOT NULL,
- starttime INTEGER NOT NULL,
+ starttime INTEGER NOT NULL, -- REVIEW: why not start_time?
stoptime INTEGER NOT NULL,
FOREIGN KEY (derivation) REFERENCES Derivations (derivation),
FOREIGN KEY (evaluation) REFERENCES Evaluations (id)
@@ -75,5 +78,12 @@ CREATE TABLE Builds (
CREATE INDEX Builds_Derivations_index ON Builds(status ASC, timestamp ASC, id, derivation, evaluation, stoptime DESC);
CREATE INDEX Inputs_index ON Inputs(specification, name, branch);
CREATE INDEX Derivations_index ON Derivations(derivation, evaluation, job_name, system);
+-- COMMENT: Indices make the following trade off: slow down writes and take
+-- more space (disk and memory) and in prize you get faster reads.
+-- COMMENT: you'd better add CREATE INDEX just below the table that they
+-- speed up queries for.
+
COMMIT;
+
+--
