diff mbox

[bug#58623,0/3] import/cran: Parameterize for guix-cran.

Message ID Y0/DVVjwYCtiDRxd@noor.fritz.box
State New
Headers show

Commit Message

Lars-Dominik Braun Oct. 19, 2022, 9:28 a.m. UTC 
Hi,

the attached patches are required for guix-cran
(https://github.com/guix-science/guix-cran). import/cran already has the
ability to add a prefix to licenses, but it was not exposed. Additionally
I need to parameterize fetch/download functions, so I can cache the
tarballs/DESCRIPTION files.

Cheers,
Lars


Lars-Dominik Braun (3):
  import/cran: Allow custom license prefix.
  import/cran: Allow overriding description fetch function.
  import/cran: Allow overriding tarball download.

 guix/import/cran.scm         | 30 +++++++++++++++++++++---------
 guix/scripts/import/cran.scm | 18 ++++++++++++++++--
 2 files changed, 37 insertions(+), 11 deletions(-)

Comments

Ludovic Courtès Nov. 5, 2022, 8:47 p.m. UTC | #1 
Hi!

Lars-Dominik Braun <lars@6xq.net> skribis:

> the attached patches are required for guix-cran
> (https://github.com/guix-science/guix-cran). import/cran already has the
> ability to add a prefix to licenses, but it was not exposed. Additionally
> I need to parameterize fetch/download functions, so I can cache the
> tarballs/DESCRIPTION files.

Nice!  Minor comments:

> +(define %license-prefix
> +  (make-parameter identity))

Overall, unless it’s impractical, I’d suggest using explicit keyword
parameters instead of SRFI-39 parameters (like this one).  That makes
things clearer.  (That’s essentially lexical scope vs. dynamic scope.)

Also, when introducing a public global variable, please add a short
comment below the ‘define’ line explaining what it does.

> +++ b/guix/scripts/import/cran.scm
> @@ -53,6 +53,9 @@ (define (show-help)
>    (display (G_ "
>    -s, --style=STYLE      choose output style, either specification or variable"))
>    (display (G_ "
> +  -p, --license-prefix=PREFIX
> +                         add custom prefix to licenses, useful for prefixed import of (guix licenses)"))

I agree with zimoun that this should be documented in the manual.  I’d
also remove everything after the comma.

> +(define %fetch-description
> +  (make-parameter fetch-description))

Same comment as above.

> +(define %download-source
> +  (make-parameter download))

Ditto.

Thanks!

Ludo’.
Ricardo Wurmus Dec. 31, 2022, 1:49 p.m. UTC | #2 
Thank you for the patches!  I made minor changes to the commit messages
(to replace “parameter” with “argument”), used multiple values with
SRFI-71 instead of cdr+cadr, adjusted expectations in a test, and fixed
a small bug in a follow-up commit.
diff mbox

Patch

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index 17e33d5f52..d13231f633 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -55,6 +55,7 @@  (define-module (guix import cran)
   #:use-module (guix packages)
   #:use-module (gnu packages)
   #:export (%input-style
+            %license-prefix
 
             cran->guix-package
             bioconductor->guix-package
@@ -82,6 +83,9 @@  (define-module (guix import cran)
 (define %input-style
   (make-parameter 'variable)) ; or 'specification
 
+(define %license-prefix
+  (make-parameter identity))
+
 (define (string->licenses license-string)
   (let ((licenses
          (map string-trim-both
@@ -89,9 +93,9 @@  (define (string->licenses license-string)
                                (char-set-complement (char-set #\|))))))
     (string->license licenses)))
 
-(define string->license
-  (let ((prefix identity))
-    (match-lambda
+(define (string->license license-string)
+  (let ((prefix (%license-prefix)))
+    (match license-string
       ("AGPL-3" (prefix 'agpl3))
       ("AGPL (>= 3)" (prefix 'agpl3+))
       ("Artistic-2.0" (prefix 'artistic2.0))
diff --git a/guix/scripts/import/cran.scm b/guix/scripts/import/cran.scm
index 2934d4300a..3186bf9248 100644
--- a/guix/scripts/import/cran.scm
+++ b/guix/scripts/import/cran.scm
@@ -53,6 +53,9 @@  (define (show-help)
   (display (G_ "
   -s, --style=STYLE      choose output style, either specification or variable"))
   (display (G_ "
+  -p, --license-prefix=PREFIX
+                         add custom prefix to licenses, useful for prefixed import of (guix licenses)"))
+  (display (G_ "
   -V, --version          display version information and exit"))
   (newline)
   (show-bug-report-information))
@@ -74,6 +77,10 @@  (define %options
                  (lambda (opt name arg result)
                    (alist-cons 'style (string->symbol arg)
                                (alist-delete 'style result))))
+         (option '(#\p "license-prefix") #t #f
+                 (lambda (opt name arg result)
+                   (alist-cons 'license-prefix arg
+                               (alist-delete 'license-prefix result))))
          (option '(#\r "recursive") #f #f
                  (lambda (opt name arg result)
                    (alist-cons 'recursive #t result)))
@@ -95,8 +102,15 @@  (define (parse-options)
                             (('argument . value)
                              value)
                             (_ #f))
-                           (reverse opts))))
-    (parameterize ((%input-style (assoc-ref opts 'style)))
+                           (reverse opts)))
+         (prefix (assoc-ref opts 'license-prefix))
+         (prefix-proc (if (string? prefix)
+                        (lambda (symbol)
+                          (string->symbol
+                            (string-append prefix (symbol->string symbol))))
+                        identity)))
+    (parameterize ((%input-style (assoc-ref opts 'style))
+                   (%license-prefix prefix-proc))
       (match args
         ((spec)
          (let ((name version (package-name->name+version spec)))