Le 2018-06-10 23:28, ludo@gnu.org a écrit :
> Salut Julien!
> 
> Julien Lepiller <julien@lepiller.eu> skribis:
> 
>> From a5250186722305961f0a5d77cb8f7f36cdae0da0 Mon Sep 17 00:00:00 2001
>> From: Julien Lepiller <julien@lepiller.eu>
>> Date: Wed, 6 Jun 2018 19:14:39 +0200
>> Subject: [PATCH] guix: Add opam importer.
>> 
>> * guix/scripts/import.scm (importers): Add opam.
>> * guix/scripts/import/opam.scm: New file.
>> * guix/import/opam.scm: New file.
>> * Makefile.am: Add them.
> 
> Very nice!  I hope that’ll help create bridges with more fellow OCaml
> hackers.  :-)
> 
> I have some comments, mostly about style:
> 
>> +(define (htable-update htable line)
>> +  "Parse @var{line} to get the name and version of the package and 
>> adds them
>> +to the hashtable."
>> +  (let* ((line (string-split line #\ ))
>> +         (url (car line)))
>> +    (unless (equal? url "repo")
>> +      (let ((sp (string-split url #\/)))
>> +        (when (equal? (car sp) "packages")
>> +          (let* ((versionstr (car (cdr (cdr sp))))
>> +                 (name1 (car (cdr sp)))
>> +                 (name2 (car (string-split versionstr #\.)))
>> +                 (version (string-join (cdr (string-split versionstr 
>> #\.)) ".")))
>> +            (when (equal? name1 name2)
>> +              (let ((curr (hash-ref htable name1 '())))
>> +                (hash-set! htable name1 (cons version curr))))))))))
> 
> There are a couple of things that don’t fully match the coding style
> (see 
> <https://www.gnu.org/software/guix/manual/html_node/Coding-Style.html>):
> try to use full names for identifiers, favor a functional style (here
> maybe you could use a vhash¹ instead of a hash table), and, last but 
> not
> least, use ‘match’ instead of ‘car’ and ‘cdr’.  :-)
> 
> ¹ https://www.gnu.org/software/guile/manual/html_node/VHashes.html
> 
>> +(define (fetch-url uri)
>> +  "Fetch and parse the url file.  Return the URL the package can be 
>> downloaded
>> +from."
> 
> Maybe ‘fetch-url-list’ or ‘fetch-package-urls’?
> 
>> +(define (fetch-metadata uri)
>> +  "Fetch and parse the opam file.  Return an association list 
>> containing the
>> +homepage, the license and the list of inputs."
> 
> Maybe ‘fetch-package-metadata’ to clarify that it’s per-package?
> 
>> +(define (deps->inputs deps)
>> +  "Transform the list of dependencies in a list of inputs.  Filter 
>> out anything
>> +that looks like a native-input."
> 
> So that would be ‘dependencies->inputs’.  :-)
> 
>> +  (if (eq? deps #f)
> 
> Rather: (if (not dependencies) …)
> 
>> +    (let ((inputs
>> +            (map (lambda (input)
>> +                   (list input (list 'unquote (string->symbol 
>> input))))
>> +              (map (lambda (input)
>> +                     (cond
>> +                       ((equal? input "ocamlfind") "ocaml-findlib")
>> +                       ((string-prefix? "ocaml" input) input)
>> +                       (else (string-append "ocaml-" input))))
>> +                (filter (lambda (input) (not (string-prefix? "conf-" 
>> input))) deps)))))
> 
> The indentation is misleading: the 2nd argument to ‘map’ should be
> aligned with the 1st.
> 
> Perhaps you can use ‘filter-map’ here?
> 
>> +      (if (eq? (length inputs) 0) '() inputs))))
> 
> (eq? (length inputs) 0) → (null? inputs)
> 
> Note that ‘null?’ is constant-time whereas ‘length’ is O(n).
> 
> Could you add:
> 
>   • A few lines in guix.texi, under “Invoking guix import”;
> 
>   • Tests in tests/opam.scm (you can take a look at tests/cpan.scm,
>     tests/elpa.scm, etc. for inspiration.)
> 
> Thank you!
> 
> Ludo’.


Here is a new version. What do you think?