[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
L
L
Ludovic Courtès wrote on 26 Jun 23:32 +0200
(name . Herman Rimm)(address . herman@rimm.ee)
87y16r9yqy.fsf@gnu.org
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@gnu.org> skribis:

Toggle quote (81 lines)
> Herman Rimm <herman@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’.
H
H
Herman Rimm wrote on 4 Jul 17:55 +0200
[PATCH v2] scripts: style: Add 'alphabetical-sort' option.
(address . 70800@debbugs.gnu.org)
20240704155507.20383-1-herman@rimm.ee
* 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(-)

Toggle diff (146 lines)
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@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,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
L
L
Ludovic Courtès wrote on 18 Jul 17:12 +0200
(name . Herman Rimm)(address . herman@rimm.ee)(address . 70800@debbugs.gnu.org)
87wmlivimk.fsf@gnu.org
Hi Herman,

Herman Rimm <herman@rimm.ee> skribis:

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