[bug#31736] [PATCH] Add an opam importer

[ludo@gnu.org (Ludovic Courtès)]

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’.