David Thompson <dthomps...@worcester.edu> 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 <dthomps...@worcester.edu> > 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’.
