Re: [PATCH] import: Add PyPI importer.

[Ludovic Courtès]

David Thompson <address@hidden> skribis:

> I spent my day working on generalizing the 'guix import' UI to allow for
> using a PyPI importer in addition to the pre-existing Nix importer.
> It's now at the point where I stop coding and open it up for review. :)

Heheh, cool! :-)

Overall looks good to me.  Some comments below:

> From b3ec259fd097034631cf311040af7aa12f7c5ebc Mon Sep 17 00:00:00 2001
> From: David Thompson <address@hidden>
> Date: Sat, 27 Sep 2014 10:16:23 -0400
> Subject: [PATCH] import: Add PyPI importer.
>
> * guix/snix.scm: Delete.
> * guix/import/snix.scm: New file.
> * guix/scripts/import/nix.scm: New file.

This is good.

> * guix/import/pypi.scm: New file.
> * guix/scripts/import/pypi.scm: New file.

Nice too.  I wonder if there may be shared options between all the
importers (like an option for import & live build.)

That can still be addressed by exporting an option list from (guix
scripts import), like (guix scripts build) does, I think.

> * Makefile.am (MODULES): Add new files and remove 'guix/snix.scm'.
> * guix/scripts/import.scm (%default-options, %options): Delete.
>   (importers): New variable.
>   (show-help): List importers.
>   (guix-import): Factor out Nix-specific logic.  Delegate to correct importer
>   based upon first argument.

Make sure to adjust tests/snix.scm.

Also, it’d be cool to have tests for (guix import pypi), at least for
the part that is concerned with parsing JSON and producing a package
object.

> +(define tarball-url->string-append
> +  (let ((tar.gz-regex (make-regexp "\\.tar\\.gz$"))
> +        (tarball-regex (make-regexp ".*-(.*)\\.tar\\.gz")))
> +    (lambda (url name version)
> +      "Return a `string-append' s-expression used for building a generic form
> +of URL for the package NAME where VERSION is replaced by a `version'
> +variable."

This is similar to what snix has, and i think it should be shared (see
below.)

> +(define (make-pypi-sexp name version source-url home-page synopsis
> +                        description license)
> +  "Return the `package' s-expression for a python package with the given 
> NAME,

Namely, what do you think of having importers return directly a
‘package’ object?  Then there could be a shared ‘package->sexp’
procedure, that would to the fancy ‘string-append’ thing like above.

And, eventually, we can add an option to do live builds of the generated
package objects.

That can also be done in the next iteration, though.

> +(define (factorize-uri uri version)
> +  "Factorize URI, a package tarball URI as a string, such that any 
> occurrences
> +of the string VERSION is replaced by the symbol 'version."

This one from snix is redundant with ‘tarball-url->string-append’ (and
maybe less sophisticated?).

Thanks!

Ludo’.