Salut Julien!
Julien Lepiller <address@hidden> skribis:
From a5250186722305961f0a5d77cb8f7f36cdae0da0 Mon Sep 17 00:00:00 2001
From: Julien Lepiller <address@hidden>
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’.
0001-guix-Add-opam-importer.patch