Re: Patches: Progressive Enhancement

[Ludovic Courtès]

Hi Alex,

Alex Sassmannshausen <address@hidden> skribis:

> The first patch tidies the CSS in (insert-css).
>
> The second patch uses fold-values from (sxml fold) to achieve what my
> previous patch did without the use of set! (essentially change the page
> so that it does not assume JavaScript to display all content elegantly).
>
> Output is visible at www.atheia.org/guix/list-packages.html again, and
> it validates correctly.

Looks great!

> From 361b7aa0eff3681a8b0a47b7139fb7e84d7c8f71 Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <address@hidden>
> Date: Mon, 26 Aug 2013 15:53:38 +0200
> Subject: [PATCH 1/2] list-packages: Tidy CSS in preparation for split into
>  external file.
>
> * build-aux/list-packages.scm (insert-css): Tidy CSS alignment etc.

Applied.

> From ed5771c1d5a18c31634ef075d6fe1eee70b32d6b Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <address@hidden>
> Date: Mon, 26 Aug 2013 15:55:28 +0200
> Subject: [PATCH 2/2] list-packages: Progressive Enhancement approach to JS.
>
> * build-aux/list-packages.scm (package->sxml): Modify formal params,
>   docstring. Introduce logic for fold-values process.

Instead of “Modify...”, please write “Add parameters foo and bar.”.

> --- a/build-aux/list-packages.scm
> +++ b/build-aux/list-packages.scm
> @@ -29,6 +29,7 @@ exec guile -l "$0"                              \
>    #:use-module (guix gnu-maintenance)
>    #:use-module (gnu packages)
>    #:use-module (sxml simple)
> +  #:use-module (sxml fold)
>    #:use-module (web uri)
>    #:use-module (ice-9 match)
>    #:use-module (srfi srfi-1)
> @@ -48,8 +49,10 @@ exec guile -l "$0"                              \
>                (equal? (gnu-package-name package) name))
>              gnu))))
>  
> -(define (package->sxml package)
> -  "Return HTML-as-SXML representing PACKAGE."
> +(define (package->sxml package previous lid l)
> +  "Return built HTML-as-SXML representing PACKAGEs, collected in
> +PREVIOUS. Include a call to the JavaScript prep_pkg_descs function, every 
> time
> +the length of LID (increasing) is 15 or when L (decreasing) is 1."

It should be “Return 3 values: the HTML-as-SXML for PACKAGE, etc.”

Also, PREVIOUS is a list of previously processed packages right?  That
should be mentioned in the docstring.

A more descriptive name for ‘lid’ would be ‘description-ids’, and its
type should be mentioned in the docstring.

‘l’ could be renamed to ‘remaining’, since it’s a count of remaining
packages, IIUC.

> +  (define (insert-tr description-id js?)
> +    (define (insert-js-call lid)
> +      "Return an sxml call to prep_pkg_descs, with up to 15 elements of lid 
> as
> +formal parameters."
> +      (define (lid->js-argl)
> +        "Parse a Scheme list into a JavaScript style arguments list."
> +        (define (helper l)
> +          (if (null? l)
> +              ""                               ; No more args, done with 
> list.
> +              (string-append ", '" (car l) "'" ; Append a further arg.
> +                             (helper (cdr l)))))
> +
> +        (string-append "'" (car lid) "'" ; Initial arg.
> +                       (helper (cdr lid))))

This looks nicer with (ice-9 match) pattern matching and ‘string-join’:

  (define (list->js-argl)
    (match lid
      (()   ; empty list (is it possible?)
       "")
      ((lid ...)
       (string-append "'" (string-join lid "', '") "'"))))

> +    (cond ((= l 1)                               ; Last package in packages
> +           (reverse                              ; Fold has reversed packages
> +            (cons (insert-tr description-id 'js) ; Only return sxml
> +                  previous)))

This code path returns a single value, whereas the others return 3.  In
general it’s better for procedures to always return the same number of
values, to avoid confusion.

> +           ,@(fold-values package->sxml packages '() '() (length packages)))

Great that you converted it to functional style.  :-)

So I think these comments are mostly cosmetic, and then we should be
done with this patch.

Thanks,
Ludo’.