[PATCH] scripts: style: Add 'order' option to alphabetically order file.

  • Open
  • quality assurance status badge
Details
3 participants
  • Herman Rimm
  • Ludovic Courtès
  • Simon Tournier
Owner
unassigned
Submitted by
Herman Rimm
Severity
normal
H
H
Herman Rimm wrote on 6 May 12:50 +0200
(address . guix-patches@gnu.org)
8e0df2707343eaee0bf39fa06fcdd5bc5cd9fba2.1714992109.git.herman@rimm.ee
* 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(-)

Toggle diff (126 lines)
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@gnu.org>
+;;; Copyright © 2024 Herman Rimm <herman@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
L
L
Ludovic Courtès wrote on 25 May 16:14 +0200
(name . Herman Rimm)(address . herman@rimm.ee)
875xv20yhq.fsf@gnu.org
Herman Rimm <herman@rimm.ee> skribis:

Toggle quote (9 lines)
> * 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!

Toggle quote (5 lines)
> 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:

Toggle quote (3 lines)
> +(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.”

Toggle quote (3 lines)
> + ;; Group define-public with preceding blanks and defines.
> + (let* ((lst (identity

I’d drop ‘identity’.

Toggle quote (10 lines)
> + (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.

Toggle quote (6 lines)
> + (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’.

Toggle quote (4 lines)
> + (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’?

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

Oops. :-)

Toggle quote (3 lines)
> + (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’.
S
S
Simon Tournier wrote on 31 May 17:36 +0200
87ed9i3seu.fsf@gmail.com
Hi,

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

Toggle quote (7 lines)
>> 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’?

[...]

Toggle quote (8 lines)
>> + (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:

Toggle snippet (9 lines)
(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))))))))

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


Cheers,
simon
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 70800@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 70800
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch