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

Previous Next

Full log

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

Previous Next

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