[bug#42338] [PATCH 03/34] guix: Add composer-build-system.

[Julien Lepiller]

Le Fri, 18 Sep 2020 10:45:48 +0200,
Ludovic Courtès <ludo@gnu.org> a écrit :

> Hi,
> 
> Julien Lepiller <julien@lepiller.eu> skribis:
> 
> > +    (let* ((package-data (read-package-data #:filename
> > composer-file))
> > +           (scripts (match (assoc-ref package-data "scripts")
> > +                      (('@ script ...) script)
> > +                      (#f '())))
> > +           (test-script
> > +             (assoc-ref scripts test-target))
> > +           (dependencies (filter (lambda (dep) (string-contains
> > dep "/"))
> > +                                 (map car
> > +                                      (match (assoc-ref
> > package-data "require")
> > +                                        (('@ dependency ...)
> > dependency)
> > +                                        (#f '())))))
> > +           (dependencies-dev
> > +             (filter (lambda (dep) (string-contains dep "/"))
> > +                     (map car
> > +                          (match (assoc-ref package-data
> > "require-dev")
> > +                            (('@ dependency ...) dependency)
> > +                            (#f '())))))
> > +           (name (assoc-ref package-data "name")))  
> 
> This is also a case for ‘define-json-mapping’.  I suppose we could use
> Guile-JSON instead of (guix build json), no?
> 
> I think this code and similar occurrences would be less intimidating
> if we used ‘define-json-mapping’; it would make the data structures
> clearer, unlike here where one has to keep in mind what the list/tree
> looks like so they can map car/cdr around.

I think we already tried that with the node build system, but we had to
revert, because we were importing guile-json from the host side. I
don't remember the details though, so if you think it's OK now, I'll
gladly make the code look nicer :)

> 
> > +      (for-each
> > +        (lambda (input)  
> 
> Like for ‘map’, please indent on the same line:
> 
>   (for-each (lambda (input)
> 
> > +      (match test-script
> > +        ((? string? command)
> > +         (unless (equal? (system command) 0)
> > +           (throw 'failed-command command)))
> > +        (('@ (? string? command) ...)
> > +         (for-each
> > +           (lambda (c)
> > +             (unless (equal? (system c) 0)
> > +               (throw 'failed-command c)))
> > +           command))  
> 
> Use (zero? x) instead of (equal? 0 x).
> 
> Also, why not use ‘invoke’?  I this because these commands are really
> shell commands and expect things like glob patterns and tilde
> expansion? If these are not shell commands, I recommend ‘invoke’,
> which will report failures more nicely.

Here I have a single string that contains shell commands, so I don't
think I can use invoke.