[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#31736] [PATCH] Add an opam importer
From: |
Ludovic Courtès |
Subject: |
[bug#31736] [PATCH] Add an opam importer |
Date: |
Sun, 10 Jun 2018 23:28:26 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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’.