GNU bug report logs - #70800
[PATCH] scripts: style: Add 'order' option to alphabetically order file.

Previous Next

Package: guix-patches;

Reported by: Herman Rimm <herman <at> rimm.ee>

Date: Mon, 6 May 2024 10:52:02 UTC

Severity: normal

Tags: patch

Done: Ludovic Courtès <ludo <at> gnu.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 70800 in the body.
You can then email your comments to 70800 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix <at> cbaines.net, dev <at> jpoiret.xyz, ludo <at> gnu.org, othacehe <at> gnu.org, rekado <at> elephly.net, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org:
bug#70800; Package guix-patches. (Mon, 06 May 2024 10:52:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Herman Rimm <herman <at> rimm.ee>:
New bug report received and forwarded. Copy sent to guix <at> cbaines.net, dev <at> jpoiret.xyz, ludo <at> gnu.org, othacehe <at> gnu.org, rekado <at> elephly.net, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org. (Mon, 06 May 2024 10:52:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Herman Rimm <herman <at> rimm.ee>
To: guix-patches <at> gnu.org
Subject: [PATCH] scripts: style: Add 'order' option to alphabetically order
 file.
Date: Mon,  6 May 2024 12:50:34 +0200
* guix/scripts/style.scm (show-help): Describe option.
(order-packages): Add procedure.
(format-whole-file): Add 'order?' argument.
(%options): Add 'order' option.
(guix-style): Alphabetically order packages in files.
* doc/guix.texi (Invoking guix style): Document option.

Change-Id: I4aa7c0bd0b6d42529ae7d304587ffb10bf5f4006
---
Hi,

I managed to create a procedure which alphabetically sorts top-level
package definitions.  Sort is not stable as I understand it, so versions
of packages get swapped.  It works well enough in small package modules,
and should not be a problem once package versions are used in sorting.

Cheers,
Herman

 doc/guix.texi          |  6 +++++
 guix/scripts/style.scm | 52 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 1c1e0164e7..6316f6bfc9 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -15097,6 +15097,12 @@ Invoking guix style
 guix style -f /etc/config.scm
 @end example
 
+@item --order
+@itemx -o
+Place the top-level package definitions in the given files in alphabetical
+order.  This option only has an effect in combination with
+@option{--whole-file}.
+
 @item --styling=@var{rule}
 @itemx -S @var{rule}
 Apply @var{rule}, one of the following styling rules:
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 211980dc1c..ace28c1bca 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2021-2023 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2024 Herman Rimm <herman <at> rimm.ee>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -29,6 +30,7 @@
 
 (define-module (guix scripts style)
   #:autoload   (gnu packages) (specification->package fold-packages)
+  #:use-module (guix combinators)
   #:use-module (guix scripts)
   #:use-module ((guix scripts build) #:select (%standard-build-options))
   #:use-module (guix ui)
@@ -494,11 +496,43 @@ (define (package-location<? p1 p2)
 ;;; Whole-file formatting.
 ;;;
 
-(define* (format-whole-file file #:rest rest)
-  "Reformat all of FILE."
+(define (order-packages lst)
+  "Place top-level package definitions in LST in alphabetical order."
+         ;; Group define-public with preceding blanks and defines.
+  (let* ((lst (identity
+                (fold2 (lambda (expr tail head)
+                         (let ((head (cons expr head)))
+                           (match expr
+                             ((? blank?)
+                              (values tail head))
+                             (('define _ ...)
+                              (values tail head))
+                             (_ (values (cons head tail) '())))))
+                       '() '() lst)))
+         (package-name (lambda (pkg)
+                         (match pkg
+                           ((('define-public _ expr) _ ...)
+                            (match expr
+                              ((or ('package _ ('name name) _ ...)
+                                   ('package ('name name) _ ...))
+                               name)
+                              (_ #f)))
+                           (_ #f))))
+         (lst (sort lst (lambda (lst1 lst2)
+                          (let ((name1 (package-name lst1))
+                                (name2 (package-name lst2)))
+                            (and name1 name2 (string> name1 name2)))))))
+    (reverse (concatenate lst))))
+
+(define* (format-whole-file file order? #:rest rest)
+  "Reformat all of FILE. When ORDER? is true, top-level package definitions
+  are put in alphabetical order."
   (with-fluids ((%default-port-encoding "UTF-8"))
-    (let ((lst (call-with-input-file file read-with-comments/sequence
-                                     #:guess-encoding #t)))
+    (let* ((lst (call-with-input-file file read-with-comments/sequence
+                                     #:guess-encoding #t))
+           (lst (if order?
+                    (order-packages lst)
+                    lst)))
       (with-atomic-file-output file
         (lambda (port)
           (apply pretty-print-with-comments/splice port lst
@@ -526,6 +560,9 @@ (define %options
         (option '(#\f "whole-file") #f #f
                 (lambda (opt name arg result)
                   (alist-cons 'whole-file? #t result)))
+        (option '(#\o "order") #f #f
+                (lambda (opt name arg result)
+                  (alist-cons 'order? #t result)))
         (option '(#\S "styling") #t #f
                 (lambda (opt name arg result)
                   (alist-cons 'styling-procedure
@@ -569,7 +606,7 @@ (define (show-help)
   (display (G_ "
   -S, --styling=RULE     apply RULE, a styling rule"))
   (display (G_ "
-  -l, --list-stylings   display the list of available style rules"))
+  -l, --list-stylings    display the list of available style rules"))
   (newline)
   (display (G_ "
   -n, --dry-run          display files that would be edited but do nothing"))
@@ -584,6 +621,8 @@ (define (show-help)
   (newline)
   (display (G_ "
   -f, --whole-file       format the entire contents of the given file(s)"))
+  (display (G_ "
+  -o, --order            place the contents in alphabetical order as well"))
   (newline)
   (display (G_ "
   -h, --help             display this help and exit"))
@@ -627,7 +666,8 @@ (define-command (guix-style . args)
               (warning (G_ "'--styling' option has no effect in whole-file mode~%")))
             (when (null? files)
               (warning (G_ "no files specified, nothing to do~%")))
-            (for-each format-whole-file files))
+            (for-each format-whole-file
+                      files (map (const (assoc-ref opts 'order?)) files)))
           (let ((packages (filter-map (match-lambda
                                         (('argument . spec)
                                          (specification->package spec))

base-commit: ef8ab6ab66c4d629699d175938ef1912941d4dce
-- 
2.41.0





Information forwarded to guix-patches <at> gnu.org:
bug#70800; Package guix-patches. (Sat, 25 May 2024 14:16:01 GMT) Full text and rfc822 format available.

Message #8 received at 70800 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Herman Rimm <herman <at> rimm.ee>
Cc: 70800 <at> debbugs.gnu.org, Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#70800] [PATCH] scripts: style: Add 'order' option to
 alphabetically order file.
Date: Sat, 25 May 2024 16:14:57 +0200
Herman Rimm <herman <at> rimm.ee> skribis:

> * guix/scripts/style.scm (show-help): Describe option.
> (order-packages): Add procedure.
> (format-whole-file): Add 'order?' argument.
> (%options): Add 'order' option.
> (guix-style): Alphabetically order packages in files.
> * doc/guix.texi (Invoking guix style): Document option.
>
> Change-Id: I4aa7c0bd0b6d42529ae7d304587ffb10bf5f4006

Yay!

> I managed to create a procedure which alphabetically sorts top-level
> package definitions.  Sort is not stable as I understand it, so versions
> of packages get swapped.  It works well enough in small package modules,
> and should not be a problem once package versions are used in sorting.

Maybe use ‘stable-sort’ instead of ‘sort’?

Overall LGTM; some suggestions below:

> +(define (order-packages lst)
> +  "Place top-level package definitions in LST in alphabetical order."

“Return LST, a list of top-level expressions and blanks, with top-level
package definitions in alphabetical order.”

> +         ;; Group define-public with preceding blanks and defines.
> +  (let* ((lst (identity

I’d drop ‘identity’.

> +         (package-name (lambda (pkg)
> +                         (match pkg
> +                           ((('define-public _ expr) _ ...)
> +                            (match expr
> +                              ((or ('package _ ('name name) _ ...)
> +                                   ('package ('name name) _ ...))
> +                               name)
> +                              (_ #f)))
> +                           (_ #f))))

Nitpick: I’d make this an inner ‘define’ in ‘order-packages’, right
below the docstring.

> +         (lst (sort lst (lambda (lst1 lst2)
> +                          (let ((name1 (package-name lst1))
> +                                (name2 (package-name lst2)))
> +                            (and name1 name2 (string> name1 name2)))))))
> +    (reverse (concatenate lst))))

Maybe replace ‘string>’ by ‘string<?’ and drop ‘reverse’.

> +        (option '(#\o "order") #f #f
> +                (lambda (opt name arg result)
> +                  (alist-cons 'order? #t result)))

I’d avoid ‘-o’ for this because it’s usually synonymous with ‘output’.

But maybe make it ‘-A’/‘--alphabetical-sort’?

>    (display (G_ "
> -  -l, --list-stylings   display the list of available style rules"))
> +  -l, --list-stylings    display the list of available style rules"))

Oops.  :-)

> +            (for-each format-whole-file
> +                      files (map (const (assoc-ref opts 'order?)) files)))

I’d go with something less inventive here:

  (for-each (cute format-whole-file <> (assoc-ref opts 'order?))
            files)

Could you send an updated patch?

Thank you!

Ludo’.




Information forwarded to guix-patches <at> gnu.org:
bug#70800; Package guix-patches. (Fri, 31 May 2024 16:38:03 GMT) Full text and rfc822 format available.

Message #11 received at 70800 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>, Herman Rimm
 <herman <at> rimm.ee>
Cc: 70800 <at> debbugs.gnu.org, Josselin Poiret <dev <at> jpoiret.xyz>,
 Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Ricardo Wurmus <rekado <at> elephly.net>, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#70800] [PATCH] scripts: style: Add 'order' option to
 alphabetically order file.
Date: Fri, 31 May 2024 17:36:09 +0200
Hi,

On Sat, 25 May 2024 at 16:14, Ludovic Courtès <ludo <at> gnu.org> wrote:

>> I managed to create a procedure which alphabetically sorts top-level
>> package definitions.  Sort is not stable as I understand it, so versions
>> of packages get swapped.  It works well enough in small package modules,
>> and should not be a problem once package versions are used in sorting.
>
> Maybe use ‘stable-sort’ instead of ‘sort’?

[...]

>> +         (lst (sort lst (lambda (lst1 lst2)
>> +                          (let ((name1 (package-name lst1))
>> +                                (name2 (package-name lst2)))
>> +                            (and name1 name2 (string> name1 name2)))))))
>> +    (reverse (concatenate lst))))
>
> Maybe replace ‘string>’ by ‘string<?’ and drop ‘reverse’.

I would suggest to use ’sort!’ for an in-place sort.  This would avoid
some GC cycles when internally copying since ’lst’ is inside ’let*’.

Moreover, yeah reverse the inequality would avoid the ’reverse’
call. :-)

About the stability, I would suggest something as:

--8<---------------cut here---------------start------------->8---
(sort! lst (lambda (lst1 lst2)
             (let ((name1 (package-name lst1))
                   (name2 (package-name lst2)))
               (and name1 name2 (or (string<? name1 name2)
                                    (eq? '< (version-compare
                                             (package-version lst1)
                                             (package-version lst2))))))))
--8<---------------cut here---------------end--------------->8---

with ’version-compare’ from (guix utils).  Well, something like
that. :-)


Cheers,
simon




Information forwarded to guix-patches <at> gnu.org:
bug#70800; Package guix-patches. (Wed, 26 Jun 2024 21:33:01 GMT) Full text and rfc822 format available.

Message #14 received at 70800 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Herman Rimm <herman <at> rimm.ee>
Cc: 70800 <at> debbugs.gnu.org, Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#70800] [PATCH] scripts: style: Add 'order' option to
 alphabetically order file.
Date: Wed, 26 Jun 2024 23:32:05 +0200
Hi Herman,

Did you have a chance to look into this?  Let us know if anything looks
unclear or questionable to you.

Thanks,
Ludo’.

Ludovic Courtès <ludo <at> gnu.org> skribis:

> Herman Rimm <herman <at> rimm.ee> skribis:
>
>> * guix/scripts/style.scm (show-help): Describe option.
>> (order-packages): Add procedure.
>> (format-whole-file): Add 'order?' argument.
>> (%options): Add 'order' option.
>> (guix-style): Alphabetically order packages in files.
>> * doc/guix.texi (Invoking guix style): Document option.
>>
>> Change-Id: I4aa7c0bd0b6d42529ae7d304587ffb10bf5f4006
>
> Yay!
>
>> I managed to create a procedure which alphabetically sorts top-level
>> package definitions.  Sort is not stable as I understand it, so versions
>> of packages get swapped.  It works well enough in small package modules,
>> and should not be a problem once package versions are used in sorting.
>
> Maybe use ‘stable-sort’ instead of ‘sort’?
>
> Overall LGTM; some suggestions below:
>
>> +(define (order-packages lst)
>> +  "Place top-level package definitions in LST in alphabetical order."
>
> “Return LST, a list of top-level expressions and blanks, with top-level
> package definitions in alphabetical order.”
>
>> +         ;; Group define-public with preceding blanks and defines.
>> +  (let* ((lst (identity
>
> I’d drop ‘identity’.
>
>> +         (package-name (lambda (pkg)
>> +                         (match pkg
>> +                           ((('define-public _ expr) _ ...)
>> +                            (match expr
>> +                              ((or ('package _ ('name name) _ ...)
>> +                                   ('package ('name name) _ ...))
>> +                               name)
>> +                              (_ #f)))
>> +                           (_ #f))))
>
> Nitpick: I’d make this an inner ‘define’ in ‘order-packages’, right
> below the docstring.
>
>> +         (lst (sort lst (lambda (lst1 lst2)
>> +                          (let ((name1 (package-name lst1))
>> +                                (name2 (package-name lst2)))
>> +                            (and name1 name2 (string> name1 name2)))))))
>> +    (reverse (concatenate lst))))
>
> Maybe replace ‘string>’ by ‘string<?’ and drop ‘reverse’.
>
>> +        (option '(#\o "order") #f #f
>> +                (lambda (opt name arg result)
>> +                  (alist-cons 'order? #t result)))
>
> I’d avoid ‘-o’ for this because it’s usually synonymous with ‘output’.
>
> But maybe make it ‘-A’/‘--alphabetical-sort’?
>
>>    (display (G_ "
>> -  -l, --list-stylings   display the list of available style rules"))
>> +  -l, --list-stylings    display the list of available style rules"))
>
> Oops.  :-)
>
>> +            (for-each format-whole-file
>> +                      files (map (const (assoc-ref opts 'order?)) files)))
>
> I’d go with something less inventive here:
>
>   (for-each (cute format-whole-file <> (assoc-ref opts 'order?))
>             files)
>
> Could you send an updated patch?
>
> Thank you!
>
> Ludo’.




Information forwarded to guix-patches <at> gnu.org:
bug#70800; Package guix-patches. (Thu, 04 Jul 2024 15:56:02 GMT) Full text and rfc822 format available.

Message #17 received at 70800 <at> debbugs.gnu.org (full text, mbox):

From: Herman Rimm <herman <at> rimm.ee>
To: 70800 <at> debbugs.gnu.org
Subject: [PATCH v2] scripts: style: Add 'alphabetical-sort' option.
Date: Thu,  4 Jul 2024 17:55:06 +0200
* guix/scripts/style.scm (show-help): Describe option.
(order-packages): Add procedure.
(format-whole-file): Add 'order?' argument.
(%options): Add 'alphabetical-sort' option.
(guix-style): Alphabetically order packages in files.
* doc/guix.texi (Invoking guix style): Document option.

Change-Id: I4aa7c0bd0b6d42529ae7d304587ffb10bf5f4006
---
Hi,

I should have mentioned it back then, but in May and again today I tried
to not use a reverse.  If you can do a sort without it, please amend the
patch.  This revision takes the other feedback into account.

Cheers,
Herman

 doc/guix.texi          |  7 ++++
 guix/scripts/style.scm | 73 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 6770c9d664..3e598f812b 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -15190,6 +15190,13 @@ configuration (you need write permissions for the file):
 guix style -f /etc/config.scm
 @end example
 
+@item --alphabetical-sort
+@itemx -A
+Place the top-level package definitions in the given files in
+alphabetical order.  Package definitions with matching names are placed
+with versions in descending order.  This option only has an effect in
+combination with @option{--whole-file}.
+
 @item --styling=@var{rule}
 @itemx -S @var{rule}
 Apply @var{rule}, one of the following styling rules:
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 0727ac1480..5f4ee4a492 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2021-2024 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2024 Herman Rimm <herman <at> rimm.ee>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -29,6 +30,7 @@
 
 (define-module (guix scripts style)
   #:autoload   (gnu packages) (specification->package fold-packages)
+  #:use-module (guix combinators)
   #:use-module (guix scripts)
   #:use-module ((guix scripts build) #:select (%standard-build-options))
   #:use-module (guix ui)
@@ -494,11 +496,62 @@ (define (package-location<? p1 p2)
 ;;; Whole-file formatting.
 ;;;
 
-(define* (format-whole-file file #:rest rest)
-  "Reformat all of FILE."
+(define (order-packages lst)
+  "Return LST, a list of top-level expressions and blanks, with
+top-level package definitions in alphabetical order.  Packages which
+share a name are placed with versions in descending order."
+  (define (package-name pkg)
+    (match pkg
+      ((('define-public _ expr) _ ...)
+       (match expr
+         ((or ('package _ ('name name) _ ...)
+              ('package ('name name) _ ...))
+          name)
+         (_ #f)))
+      (_ #f)))
+
+  (define (package-version pkg)
+    (match pkg
+      ((('define-public _ expr) _ ...)
+       (match expr
+         ((or ('package _ _ ('version version) _ ...)
+              ('package _ ('version version) _ ...))
+          version)
+         (_ #f)))
+      (_ #f)))
+
+  (define (package>? lst1 lst2)
+    (let ((name1 (package-name lst1))
+          (name2 (package-name lst2))
+          (version1 (package-version lst1))
+          (version2 (package-version lst2)))
+      (and name1 name2 (or (string>? name1 name2)
+                           (and (string=? name1 name2)
+                                version1
+                                version2
+                                (version>? version2 version1))))))
+
+        ;; Group define-public with preceding blanks and defines.
+  (let ((lst (fold2 (lambda (expr tail head)
+                      (let ((head (cons expr head)))
+                        (match expr
+                          ((? blank?)
+                           (values tail head))
+                          (('define _ ...)
+                           (values tail head))
+                          (_ (values (cons head tail) '())))))
+                    '() '() lst)))
+    (reverse (concatenate (sort! lst package>?)))))
+
+(define* (format-whole-file file order? #:rest rest)
+  "Reformat all of FILE. When ORDER? is true, top-level package definitions
+are put in alphabetical order."
   (with-fluids ((%default-port-encoding "UTF-8"))
-    (let ((lst (call-with-input-file file read-with-comments/sequence
-                                     #:guess-encoding #t)))
+    (let* ((lst (call-with-input-file file read-with-comments/sequence
+                                      #:guess-encoding #t))
+           (lst (if order?
+                    (order-packages lst)
+                    lst)))
       (with-atomic-file-output file
         (lambda (port)
           (apply pretty-print-with-comments/splice port lst
@@ -526,6 +579,9 @@ (define %options
         (option '(#\f "whole-file") #f #f
                 (lambda (opt name arg result)
                   (alist-cons 'whole-file? #t result)))
+        (option '(#\A "--alphabetical-sort") #f #f
+                (lambda (opt name arg result)
+                  (alist-cons 'order? #t result)))
         (option '(#\S "styling") #t #f
                 (lambda (opt name arg result)
                   (alist-cons 'styling-procedure
@@ -569,7 +625,7 @@ (define (show-help)
   (display (G_ "
   -S, --styling=RULE     apply RULE, a styling rule"))
   (display (G_ "
-  -l, --list-stylings   display the list of available style rules"))
+  -l, --list-stylings    display the list of available style rules"))
   (newline)
   (display (G_ "
   -n, --dry-run          display files that would be edited but do nothing"))
@@ -584,6 +640,9 @@ (define (show-help)
   (newline)
   (display (G_ "
   -f, --whole-file       format the entire contents of the given file(s)"))
+  (display (G_ "
+  -A, --alphabetical-sort
+                         place the contents in alphabetical order as well"))
   (newline)
   (display (G_ "
   -h, --help             display this help and exit"))
@@ -627,7 +686,9 @@ (define (parse-options)
               (warning (G_ "'--styling' option has no effect in whole-file mode~%")))
             (when (null? files)
               (warning (G_ "no files specified, nothing to do~%")))
-            (for-each format-whole-file files))
+            (for-each
+              (cute format-whole-file <> (assoc-ref opts 'order?))
+              files))
           (let ((packages (filter-map (match-lambda
                                         (('argument . spec)
                                          (specification->package spec))
-- 
2.45.1





Information forwarded to guix-patches <at> gnu.org:
bug#70800; Package guix-patches. (Thu, 18 Jul 2024 15:14:02 GMT) Full text and rfc822 format available.

Message #20 received at 70800 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Herman Rimm <herman <at> rimm.ee>
Cc: 70800 <at> debbugs.gnu.org
Subject: Re: [bug#70800] [PATCH v2] scripts: style: Add 'alphabetical-sort'
 option.
Date: Thu, 18 Jul 2024 17:12:51 +0200
Hi Herman,

Herman Rimm <herman <at> rimm.ee> skribis:

> * guix/scripts/style.scm (show-help): Describe option.
> (order-packages): Add procedure.
> (format-whole-file): Add 'order?' argument.
> (%options): Add 'alphabetical-sort' option.
> (guix-style): Alphabetically order packages in files.
> * doc/guix.texi (Invoking guix style): Document option.
>
> Change-Id: I4aa7c0bd0b6d42529ae7d304587ffb10bf5f4006
> ---
> Hi,
>
> I should have mentioned it back then, but in May and again today I tried
> to not use a reverse.  If you can do a sort without it, please amend the
> patch.  This revision takes the other feedback into account.

This looks great to me, thanks!  I realize I forgot to ask you to add
tests to ‘tests/guix-style.sh’—my bad.

Would you be willing and able to submit an updated version with tests?
If not (which I’d understand), I can allocate time to do it.

Thanks,
Ludo’.




Information forwarded to guix-patches <at> gnu.org:
bug#70800; Package guix-patches. (Mon, 02 Sep 2024 19:00:02 GMT) Full text and rfc822 format available.

Message #23 received at 70800 <at> debbugs.gnu.org (full text, mbox):

From: Herman Rimm <herman <at> rimm.ee>
To: 70800 <at> debbugs.gnu.org
Subject: [PATCH] scripts: style: Add 'alphabetical-sort' option.
Date: Mon,  2 Sep 2024 20:58:05 +0200
* guix/scripts/style.scm (show-help): Describe option.
(order-packages): Add procedure.
(format-whole-file): Add 'order?' argument.
(%options): Add 'alphabetical-sort' option.
(guix-style): Alphabetically order packages in files.
* tests/guix-style.sh: Test alphabetical ordering.
* doc/guix.texi (Invoking guix style): Document option.

Change-Id: I4aa7c0bd0b6d42529ae7d304587ffb10bf5f4006
---
 doc/guix.texi          |  7 ++++
 guix/scripts/style.scm | 73 ++++++++++++++++++++++++++++++++++++++----
 tests/guix-style.sh    | 26 +++++++++++++++
 3 files changed, 100 insertions(+), 6 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index f7dbe246a9..6f6f952f96 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -15216,6 +15216,13 @@ configuration (you need write permissions for the file):
 guix style -f /etc/config.scm
 @end example
 
+@item --alphabetical-sort
+@itemx -A
+Place the top-level package definitions in the given files in
+alphabetical order.  Package definitions with matching names are placed
+with versions in descending order.  This option only has an effect in
+combination with @option{--whole-file}.
+
 @item --styling=@var{rule}
 @itemx -S @var{rule}
 Apply @var{rule}, one of the following styling rules:
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 1802b854d0..73e3feadee 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2021-2024 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2024 Herman Rimm <herman <at> rimm.ee>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -29,6 +30,7 @@
 
 (define-module (guix scripts style)
   #:autoload   (gnu packages) (specification->package fold-packages)
+  #:use-module (guix combinators)
   #:use-module (guix scripts)
   #:use-module ((guix scripts build) #:select (%standard-build-options))
   #:use-module (guix ui)
@@ -485,11 +487,62 @@ (define* (format-package-definition package
 ;;; Whole-file formatting.
 ;;;
 
-(define* (format-whole-file file #:rest rest)
-  "Reformat all of FILE."
+(define (order-packages lst)
+  "Return LST, a list of top-level expressions and blanks, with
+top-level package definitions in alphabetical order.  Packages which
+share a name are placed with versions in descending order."
+  (define (package-name pkg)
+    (match pkg
+      ((('define-public _ expr) _ ...)
+       (match expr
+         ((or ('package _ ('name name) _ ...)
+              ('package ('name name) _ ...))
+          name)
+         (_ #f)))
+      (_ #f)))
+
+  (define (package-version pkg)
+    (match pkg
+      ((('define-public _ expr) _ ...)
+       (match expr
+         ((or ('package _ _ ('version version) _ ...)
+              ('package _ ('version version) _ ...))
+          version)
+         (_ #f)))
+      (_ #f)))
+
+  (define (package>? lst1 lst2)
+    (let ((name1 (package-name lst1))
+          (name2 (package-name lst2))
+          (version1 (package-version lst1))
+          (version2 (package-version lst2)))
+      (and name1 name2 (or (string>? name1 name2)
+                           (and (string=? name1 name2)
+                                version1
+                                version2
+                                (version>? version2 version1))))))
+
+        ;; Group define-public with preceding blanks and defines.
+  (let ((lst (fold2 (lambda (expr tail head)
+                      (let ((head (cons expr head)))
+                        (match expr
+                          ((? blank?)
+                           (values tail head))
+                          (('define _ ...)
+                           (values tail head))
+                          (_ (values (cons head tail) '())))))
+                    '() '() lst)))
+    (reverse (concatenate (sort! lst package>?)))))
+
+(define* (format-whole-file file order? #:rest rest)
+  "Reformat all of FILE. When ORDER? is true, top-level package definitions
+are put in alphabetical order."
   (with-fluids ((%default-port-encoding "UTF-8"))
-    (let ((lst (call-with-input-file file read-with-comments/sequence
-                                     #:guess-encoding #t)))
+    (let* ((lst (call-with-input-file file read-with-comments/sequence
+                                      #:guess-encoding #t))
+           (lst (if order?
+                    (order-packages lst)
+                    lst)))
       (with-atomic-file-output file
         (lambda (port)
           (apply pretty-print-with-comments/splice port lst
@@ -517,6 +570,9 @@ (define %options
         (option '(#\f "whole-file") #f #f
                 (lambda (opt name arg result)
                   (alist-cons 'whole-file? #t result)))
+        (option '(#\A "--alphabetical-sort") #f #f
+                (lambda (opt name arg result)
+                  (alist-cons 'order? #t result)))
         (option '(#\S "styling") #t #f
                 (lambda (opt name arg result)
                   (alist-cons 'styling-procedure
@@ -560,7 +616,7 @@ (define (show-help)
   (display (G_ "
   -S, --styling=RULE     apply RULE, a styling rule"))
   (display (G_ "
-  -l, --list-stylings   display the list of available style rules"))
+  -l, --list-stylings    display the list of available style rules"))
   (newline)
   (display (G_ "
   -n, --dry-run          display files that would be edited but do nothing"))
@@ -575,6 +631,9 @@ (define (show-help)
   (newline)
   (display (G_ "
   -f, --whole-file       format the entire contents of the given file(s)"))
+  (display (G_ "
+  -A, --alphabetical-sort
+                         place the contents in alphabetical order as well"))
   (newline)
   (display (G_ "
   -h, --help             display this help and exit"))
@@ -618,7 +677,9 @@ (define (parse-options)
               (warning (G_ "'--styling' option has no effect in whole-file mode~%")))
             (when (null? files)
               (warning (G_ "no files specified, nothing to do~%")))
-            (for-each format-whole-file files))
+            (for-each
+              (cute format-whole-file <> (assoc-ref opts 'order?))
+              files))
           (let ((packages (filter-map (match-lambda
                                         (('argument . spec)
                                          (specification->package spec))
diff --git a/tests/guix-style.sh b/tests/guix-style.sh
index 2de879d5e3..9333139435 100644
--- a/tests/guix-style.sh
+++ b/tests/guix-style.sh
@@ -58,6 +58,24 @@ cat > "$tmpfile" <<EOF
   ;; The services.
   (services
    (cons (service mcron-service-type) %base-services)))
+;; Incomplete package definitions in alphabetical order.
+
+(define-public pkg
+  (package
+    (name "bar")
+    (version "2")))
+
+;; The comment below belongs to the foo package.
+(define-public pkg
+  (package
+    (name "bar")
+    (version "1")))
+;; Incomplete package definitions in alphabetical order.
+
+(define-public pkg
+  (package
+    (name "foo")
+    (version "2")))
 EOF
 
 cp "$tmpfile" "$tmpfile.bak"
@@ -78,3 +96,11 @@ test "$initial_hash" != "$(guix hash "$tmpfile")"
 
 guix style -f "$tmpfile"
 test "$initial_hash" = "$(guix hash "$tmpfile")"
+
+# Swap foo and bar packages.
+sed -i "$tmpfile" -e 's/"foo"/"bar"/g'
+sed -i "$tmpfile" -e '0,/"bar"/{s//"foo"/}'
+test "$initial_hash" != "$(guix hash "$tmpfile")"
+
+guix style -fA "$tmpfile"
+test "$initial_hash" = "$(guix hash "$tmpfile")"
-- 
2.45.2





Reply sent to Ludovic Courtès <ludo <at> gnu.org>:
You have taken responsibility. (Tue, 03 Sep 2024 09:54:01 GMT) Full text and rfc822 format available.

Notification sent to Herman Rimm <herman <at> rimm.ee>:
bug acknowledged by developer. (Tue, 03 Sep 2024 09:54:02 GMT) Full text and rfc822 format available.

Message #28 received at 70800-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Herman Rimm <herman <at> rimm.ee>
Cc: 70800-done <at> debbugs.gnu.org
Subject: Re: [bug#70800] [PATCH] scripts: style: Add 'alphabetical-sort'
 option.
Date: Tue, 03 Sep 2024 11:50:20 +0200
Hi Herman,

Herman Rimm <herman <at> rimm.ee> skribis:

> * guix/scripts/style.scm (show-help): Describe option.
> (order-packages): Add procedure.
> (format-whole-file): Add 'order?' argument.
> (%options): Add 'alphabetical-sort' option.
> (guix-style): Alphabetically order packages in files.
> * tests/guix-style.sh: Test alphabetical ordering.
> * doc/guix.texi (Invoking guix style): Document option.
>
> Change-Id: I4aa7c0bd0b6d42529ae7d304587ffb10bf5f4006

Finally applied, thank you!

Ludo’.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Wed, 02 Oct 2024 11:24:05 GMT) Full text and rfc822 format available.

This bug report was last modified 15 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.