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

["Ludovic Courtès" <ludo@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:

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