From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: GSoC: Adding a web interface similar to the Hydra web interface Date: Thu, 24 May 2018 08:03:26 +0200 Message-ID: <87tvqxy4i9.fsf@elephly.net> References: <87vac3twbe.fsf@gnu.org> <87o9hog2ye.fsf@elephly.net> <87d0xyn9zs.fsf@elephly.net> <87d0xswvls.fsf@elephly.net> <87r2m4ntk4.fsf@mdc-berlin.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:54384) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fLqcP-0008Pk-PV for guix-devel@gnu.org; Thu, 24 May 2018 09:49:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fLqcJ-0006xi-CE for guix-devel@gnu.org; Thu, 24 May 2018 09:49:25 -0400 Received: from sender-of-o51.zoho.com ([135.84.80.216]:21060) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fLqcJ-0006xU-4T for guix-devel@gnu.org; Thu, 24 May 2018 09:49:19 -0400 In-reply-to: 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: Tatiana Sholokhova Cc: guix-devel Hi Tatiana, > I have committed the first HTML template page (with table of specificatio= ns > stored in the database) to web-interface branch. Can you please review it= ? That=E2=80=99s great! Congratulations on your first commit! I=E2=80=99ll make a couple of extra comments on style and conventions becau= se this is your first commit. You aren=E2=80=99t expected to remember all of = these conventions right away. > commit a4fe6dd0d0c82c84a810d3368dd60fea3aa1b2b0 (HEAD -> web-interface, o= rigin/web-interface) > Author: TSholokhova > Date: Wed May 23 16:37:23 2018 +0300 > > basic html templates Please remember to make expressive commit messages. We normally use a format similar to the ChangeLog convention, which consists of a one-line summary that is usually a sentence, and a listing of all changes. In this case it would be something like: --8<---------------cut here---------------start------------->8--- Add basic HTML templates. * src/cuirass/templates.scm: New file. * Makefile.am (dist_pkgmodule_DATA): Add it. * src/cuirass/http.scm (url-handler): Add handler for =E2=80=9Cstatus=E2=80= =9D endpoint. --8<---------------cut here---------------end--------------->8--- > diff --git a/src/cuirass/http.scm b/src/cuirass/http.scm > index e911b9b..f5e3ac1 100644 > --- a/src/cuirass/http.scm > +++ b/src/cuirass/http.scm > @@ -1,3 +1,4 @@ > + > ;;;; http.scm -- HTTP API > ;;; Copyright =C2=A9 2016 Mathieu Lirzin > ;;; Copyright =C2=A9 2017 Mathieu Othacehe Please make sure to add a copyright line for yourself to the bottom of the copyright block. Also try to avoid pure whitespace changes like the addition of an empty line at the top of the file. > @@ -135,6 +139,12 @@ Hydra format." > #:body > (object->json-string > `((error . ,message))))) > + > + (define (respond-html body) > + (respond '((content-type . (text/html))) > + #:body (lambda (port) > + (sxml->xml body port) > + ))) Please don=E2=80=99t leave closing parentheses on a line by itself; they fe= el lonely and prefer to be on the previous line. > @@ -223,6 +233,11 @@ Hydra format." > ,@params > (order status+submission-time))))) > (respond-json-with-error 500 "Parameter not defined!")))) > + (("status") > + (respond-html (templatize > +=09=09 "Status" > +=09=09 (specifications-table > +=09=09 (with-critical-section db-channel (db) (db-get-specifications= db)))))) Here and in other places you used tabs, but we=E2=80=99re using spaces for indentation in all source files. Please configure your editor to use spaces instead of tabs. I feel that the =E2=80=9Curl-handler=E2=80=9D procedure is already very lar= ge, so it may be a good idea to break out the handler to a separate procedure instead of having the details inline. This is not a problem yet, but it may be good to keep in mind in case you need to grow this handler in the future. As long as it stays this small it=E2=80=99s fine to keep it there. > diff --git a/src/cuirass/templates.scm b/src/cuirass/templates.scm > new file mode 100644 > index 0000000..ff63469 > --- /dev/null > +++ b/src/cuirass/templates.scm > @@ -0,0 +1,32 @@ > +(define-module (cuirass templates) > + #:export (templatize > + specifications-table)) > + > + Please add the usual copyright header to the top of this module. > +(define (templatize title body) > + `(html > + ,(head title) > + (body ,body))) Please also add a docstring to every toplevel definition to explain what the procedure is supposed to do. =E2=80=9Ctemplatize=E2=80=9D is quite a f= ancy name for what I=E2=80=99d call =E2=80=9Chtml-page=E2=80=9D :) > + > +(define (head title) > + `(head > + (meta (@ (charset "utf-8"))) > + (title ,title))) This could become part of =E2=80=9Ctemplatize=E2=80=9D instead. It=E2=80= =99s generally good to have small independent procedures, but in this case I don=E2=80=99t see us = ever using =E2=80=9Chead=E2=80=9D without =E2=80=9Ctemplatize=E2=80=9D. > +(define (specifications-table specs) > + `(table > + (@ (class "table-fill")) > + (thead > + (tr > + (th (@ (class "text-left")) Name) > + (th (@ (class "text-left")) Branch))) > + (tbody > + (@ (class "table-fill")) > + ,@(map > + (lambda (spec) > + `(tr > + (td ,(assq-ref spec #:name)) > + (td ,(assq-ref spec #:branch)))) > + specs)))) Please also add a docstring to this procedure. What is the result of this procedure if =E2=80=9Cspecs=E2=80=9D is empty? Should that case be co= vered to communicate this to the viewer? > I am a bit confused about the database structure. As far as I understand, > there are project_name (project) and branch_name (jobset) properties, but > project_name is a primary key, so a project can't have several branches? I share your confusion. Maybe Ludovic or Mathieu can shed some more light on this. It is true that =E2=80=9Crepo_name=E2=80=9D (not project_name) in the Speci= fications table is the primary key and it is used as the only foreign key on other tables. On the other hand, =E2=80=9Crepo_name=E2=80=9D is really just an a= rbitrary name. You could have =E2=80=9CGuix (master branch)=E2=80=9D as the value f= or repo_name with =E2=80=9Cbranch=E2=80=9D as =E2=80=9Cmaster=E2=80=9D, and you could ha= ve another specification with the repo_name =E2=80=9CGuix (next)=E2=80=9D with =E2=80=9Cbranch=E2=80=9D a= s =E2=80=9Ccore-updates=E2=80=9D. I feel it would be nicer to have a composite primary key consisting of both the repo_name and the branch, but since the =E2=80=9Cbranch=E2=80=9D i= s an optional field maybe that=E2=80=99s not so easy. But I don=E2=80=99t really know if there are other reasons for doing it thi= s way. > I'm working on static file serving but I don't know how to set the path o= f > the static file directory. Now I just wrote my absolute path to the > style.css file. So, I have two questions. 1. Where should I place the > static files? 2. How can I execute getcwd function (as you do it in > rcas-web/rcas/config.scm.in)? I tried to add something like > > (define-public %current-directory > `(,(getcwd))) Do you really need the current directory, or could you use %datadir instead? Note that the value of %datadir is provided at configuration time, i.e. when the =E2=80=9Cconfigure=E2=80=9D script is run. =E2=80=9Cco= nfigure=E2=80=9D generates =E2=80=9Cconfig.scm=E2=80=9D from =E2=80=9Cconfig.scm.in=E2=80=9D by substi= tuting all placeholders (strings wrapped in =E2=80=9C@=E2=80=A6@=E2=80=9D) with the configured valu= es. If you want to avoid the need to reconfigure and install cuirass every time, you could respond to variables set in the =E2=80=9Cpre-inst-env=E2=80= =9D script. For example, you can see the following in src/cuirass/database.scm: --8<---------------cut here---------------start------------->8--- (define %package-schema-file ;; Define to the database schema file of this package. (make-parameter (string-append (or (getenv "CUIRASS_DATADIR") (string-append %datadir "/" %package)) "/schema.sql"))) --8<---------------cut here---------------end--------------->8--- CUIRASS_DATADIR is specified by =E2=80=9Cpre-inst-env=E2=80=9D, so when Cui= rass is run this way, it looks up the schema.sql file from the source directory. If Cuirass has been installed, however, and is not run with the =E2=80=9Cpre-inst-env=E2=80=9D script it looks up the file in the %datadir. Does this help? -- Ricardo