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

Previous Next

To reply to this bug, email your comments to 70800 AT debbugs.gnu.org.

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

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

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’.

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

Previous Next

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