diff mbox series

[bug#68935,3/3] guix: import: Put packages into modules in alphabetical order.

Message ID a380fd9adc163c8b425397163956672f5cda70c5.1707144191.git.herman@rimm.ee
State New
Headers show
Series Add 'put' option to guix import. | expand

Commit Message

Herman Rimm Feb. 5, 2024, 3:07 p.m. UTC
* guix/scripts/import.scm (guix-import): Add 'put' option.
(try-import): Add variable.
* doc/guix.texi (Invoking guix import): Describe 'put' option.

Change-Id: Ied4fc95899c31fb2523a01139780fc9744aa63cb
---
 doc/guix.texi           |  14 +++++-
 guix/scripts/import.scm | 100 +++++++++++++++++++++++++++-------------
 2 files changed, 81 insertions(+), 33 deletions(-)

Comments

Ludovic Courtès Feb. 7, 2024, 9:57 p.m. UTC | #1
Herman Rimm <herman@rimm.ee> skribis:

> * guix/scripts/import.scm (guix-import): Add 'put' option.
> (try-import): Add variable.
> * doc/guix.texi (Invoking guix import): Describe 'put' option.
>
> Change-Id: Ied4fc95899c31fb2523a01139780fc9744aa63cb

Nice!  This can certainly simplify people’s work!

>  @example
> -guix import @var{importer} @var{options}@dots{}
> +guix import @var{option} @var{importer} @var{options}@dots{}

Uh?  It looks weird at first.  :-)

For clarity, we should change it to something like:

  guix import [@var{global-options}@dots{}] @var{importer} @var{package} [@var{options}@dots{}]

… though I admit this doesn’t look great either.

> +@item --put=@var{file}
> +@itemx -p @var{file}
> +Insert the package definition(s) that the @var{importer} generated into the

How about calling it ‘--insert’?  It seems more natural.

> +(define (try-import importer args finish)

Please add a docstring for top-level procedures like this one:

  https://guix.gnu.org/manual/devel/en/html_node/Formatting-Code.html

> +  (if (member importer importers)
> +        (match (apply (resolve-importer importer) args)

Indentation of the second line is off.

> +          ((and expr ('package _ ...))
> +           (finish (package->definition expr)))
> +          ((and expr (or ('let _ ...)
> +                         ('define-public _ ...)))
> +           (finish expr))

Again (let …) should be treated like (package …).

> +          ((? list? expressions)
> +           (for-each (lambda (expr)
> +               (match expr
> +                      ((and expr ('package _ ...))
> +                       (finish (package->definition expr)))
> +                      ((and expr (or ('let _ ...)
> +                                     ('define-public _ ...)))
> +                       (finish expr))))

Likewise.  Also indentation of ‘match’ and that of the subsequent
clauses is off.

> -    ((or ("-h") ("--help"))
> +    ((or ("-h" _ ...) ("--help" _ ...))
>       (leave-on-EPIPE (show-help))
>       (exit 0))
> -    ((or ("-V") ("--version"))
> +    ((or ("-V" _ ...) ("--version" _ ...))
>       (show-version-and-exit "guix import"))

This change seems to be unrelated, right?

> +    ((or ("-p" module importer args ...)
> +         ("--put" module importer args ...))
> +     (let ((put (lambda (expr)
> +                  (let ((line
> +                    (call-with-input-file module
> +                      (lambda (port)
> +                        (do ((se (read-syntax port)
> +                                 (read-syntax port)))
> +                          ((match (syntax->datum se)
> +                             (('define-public symbol _ ...)
> +                              (string> (symbol->string symbol)
> +                                       (symbol->string (cadr expr))))
> +                             ((? eof-object?) #t)
> +                             (_ #f))
> +                           (if (eof-object? se)
> +                             #f
> +                             (car (string-split
> +                                    (object->string*
> +                                      (syntax->datum se) 0)
> +                                    #\newline)))))))))
> +                    (if line
> +                        (substitute* module
> +                          (((string-append "\\" line))
> +                           (string-append
> +                             (object->string* expr 0)
> +                             "\n\n" line)))
> +                        (let ((port (open-file module "a")))
> +                          (pretty-print-with-comments port expr)
> +                          (newline port)
> +                          (close-port port)))))))

This whole thing should be in a separate procedure, probably in (guix
utils), close to ‘edit-expression’, and we could call it
‘insert-expression’ and it would take the location where the caller
wants to insert an expression.

Then, separately, we need a procedure to determine that location based
on the alphabetical order to top-level ‘define-public’ occurrences.

Does that make sense?  We should try and add tests for these in
‘tests/utils.scm’.

Also please check out the coding style regarding use of ‘car’ etc.:

  https://guix.gnu.org/manual/devel/en/html_node/Data-Types-and-Pattern-Matching.html

Could you send an updated series?

Please let us know if anything’s unclear or if you need guidance.

Thank you!

Ludo’.
diff mbox series

Patch

diff --git a/doc/guix.texi b/doc/guix.texi
index b76df868f8..5ab4c0d2a8 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -124,6 +124,7 @@ 
 Copyright @copyright{} 2023 Saku Laesvuori@*
 Copyright @copyright{} 2023 Graham James Addis@*
 Copyright @copyright{} 2023 Tomas Volf@*
+Copyright @copyright{} 2024 Herman Rimm@*
 
 Permission is granted to copy, distribute and/or modify this document
 under the terms of the GNU Free Documentation License, Version 1.3 or
@@ -14178,12 +14179,21 @@  Invoking guix import
 The general syntax is:
 
 @example
-guix import @var{importer} @var{options}@dots{}
+guix import @var{option} @var{importer} @var{options}@dots{}
 @end example
 
 @var{importer} specifies the source from which to import package
 metadata, and @var{options} specifies a package identifier and other
-options specific to @var{importer}.
+options specific to @var{importer}. @command{guix import} itself has the
+following options:
+
+@table @code
+@item --put=@var{file}
+@itemx -p @var{file}
+Insert the package definition(s) that the @var{importer} generated into the
+specified @var{file}, either in alphabetical order among existing package
+definitions, or at the end of the file otherwise.
+@end table
 
 Some of the importers rely on the ability to run the @command{gpgv} command.
 For these, GnuPG must be installed and in @code{$PATH}; run @code{guix install
diff --git a/guix/scripts/import.scm b/guix/scripts/import.scm
index 80b1bec7fc..0d83483c4e 100644
--- a/guix/scripts/import.scm
+++ b/guix/scripts/import.scm
@@ -24,6 +24,7 @@ 
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (guix scripts import)
+  #:use-module (guix build utils)
   #:use-module (guix import utils)
   #:use-module (guix ui)
   #:use-module (guix scripts)
@@ -67,10 +68,37 @@  (define (show-help)
   (display (G_ "
   -h, --help             display this help and exit"))
   (display (G_ "
+  -p, --put              put the import in a package module"))
+  (display (G_ "
   -V, --version          display version information and exit"))
   (newline)
   (show-bug-report-information))
 
+(define (try-import importer args finish)
+  (if (member importer importers)
+        (match (apply (resolve-importer importer) args)
+          ((and expr ('package _ ...))
+           (finish (package->definition expr)))
+          ((and expr (or ('let _ ...)
+                         ('define-public _ ...)))
+           (finish expr))
+          ((? list? expressions)
+           (for-each (lambda (expr)
+               (match expr
+                      ((and expr ('package _ ...))
+                       (finish (package->definition expr)))
+                      ((and expr (or ('let _ ...)
+                                     ('define-public _ ...)))
+                       (finish expr))))
+             expressions))
+          (x
+           (leave (G_ "'~a' import failed~%") importer)))
+      (let ((hint (string-closest importer importers #:threshold 3)))
+        (report-error (G_ "~a: invalid importer~%") importer)
+        (when hint
+          (display-hint (G_ "Did you mean @code{~a}?~%") hint))
+        (exit 1))))
+
 (define-command (guix-import . args)
   (category packaging)
   (synopsis "import a package definition from an external repository")
@@ -79,38 +107,48 @@  (define-command (guix-import . args)
     (()
      (format (current-error-port)
              (G_ "guix import: missing importer name~%")))
-    ((or ("-h") ("--help"))
+    ((or ("-h" _ ...) ("--help" _ ...))
      (leave-on-EPIPE (show-help))
      (exit 0))
-    ((or ("-V") ("--version"))
+    ((or ("-V" _ ...) ("--version" _ ...))
      (show-version-and-exit "guix import"))
+    ((or ("-p" module importer args ...)
+         ("--put" module importer args ...))
+     (let ((put (lambda (expr)
+                  (let ((line
+                    (call-with-input-file module
+                      (lambda (port)
+                        (do ((se (read-syntax port)
+                                 (read-syntax port)))
+                          ((match (syntax->datum se)
+                             (('define-public symbol _ ...)
+                              (string> (symbol->string symbol)
+                                       (symbol->string (cadr expr))))
+                             ((? eof-object?) #t)
+                             (_ #f))
+                           (if (eof-object? se)
+                             #f
+                             (car (string-split
+                                    (object->string*
+                                      (syntax->datum se) 0)
+                                    #\newline)))))))))
+                    (if line
+                        (substitute* module
+                          (((string-append "\\" line))
+                           (string-append
+                             (object->string* expr 0)
+                             "\n\n" line)))
+                        (let ((port (open-file module "a")))
+                          (pretty-print-with-comments port expr)
+                          (newline port)
+                          (close-port port)))))))
+       (try-import importer args put)))
     ((importer args ...)
-     (if (member importer importers)
-         (let ((print (lambda (expr)
-                        (leave-on-EPIPE
-                         (pretty-print-with-comments (current-output-port) expr)))))
-           (match (apply (resolve-importer importer) args)
-             ((and expr ('package _ ...))
-              (print (package->definition expr)))
-             ((and expr (or ('let _ ...)
-                            ('define-public _ ...)))
-              (print expr))
-             ((? list? expressions)
-              (for-each (lambda (expr)
-                  (match expr
-                         ((and expr ('package _ ...))
-                          (print (package->definition expr)))
-                         ((and expr (or ('let _ ...)
-                                        ('define-public _ ...)))
-                          (print expr)))
-                  ;; Two newlines: one after the closing paren, and
-                  ;; one to leave a blank line.
-                  (newline) (newline))
-                expressions))
-             (x
-              (leave (G_ "'~a' import failed~%") importer))))
-         (let ((hint (string-closest importer importers #:threshold 3)))
-           (report-error (G_ "~a: invalid importer~%") importer)
-           (when hint
-             (display-hint (G_ "Did you mean @code{~a}?~%") hint))
-           (exit 1))))))
+     (let ((print (lambda (expr)
+                    (leave-on-EPIPE
+                      (pretty-print-with-comments
+                        (current-output-port) expr)
+                      ;; Two newlines: one after the closing paren,
+                      ;; and one to leave a blank line.
+                      (newline) (newline)))))
+       (try-import importer args print)))))