Re: Regarding the vertical alignment in the record definitions

[Efraim Flashner <efraim@flashner.co.il>]

[-- Attachment #1: Type: text/plain, Size: 6892 bytes --]

On Tue, Dec 03, 2024 at 01:34:29AM +0100, Tomas Volf wrote:
> 
> Hello Guix,
> 
> I would like to bring up for debate the convention the project has of
> vertically aligning the record definitions.  While I agree it lead to
> visually pleasing code, I also leads to significantly bloated diffs.
> 
> While some amount of noise in diffs is expected in any lisp language
> (the trailing `)'), the vertical aligning makes it worse.  It (in my
> opinion) complicates both reviewing and re-basing (due to merge
> conflicts).
> 
> For example, take this patch I sent today.  It adds *a single field* to
> the record.  However the patch (that someone needs to review and make
> sure is correct) looks like this:
> 
> --8<---------------cut here---------------start------------->8---
> @@ -1246,44 +1247,49 @@ (define-deprecated (agetty-service config)
>  (define-record-type* <mingetty-configuration>
>    mingetty-configuration make-mingetty-configuration
>    mingetty-configuration?
> -  (mingetty         mingetty-configuration-mingetty ;file-like
> -                    (default mingetty))
> -  (tty              mingetty-configuration-tty)       ;string
> -  (auto-login       mingetty-configuration-auto-login ;string | #f
> -                    (default #f))
> -  (login-program    mingetty-configuration-login-program ;gexp
> -                    (default #f))
> -  (login-pause?     mingetty-configuration-login-pause? ;Boolean
> -                    (default #f))
> -  (clear-on-logout? mingetty-configuration-clear-on-logout? ;Boolean
> -                    (default #t))
> -  (delay            mingetty-configuration-delay ;Integer | #f
> -                    (default #f))
> -  (print-issue      mingetty-configuration-print-issue ;Boolean | Symbol
> -                    (default #t))
> -  (print-hostname   mingetty-configuration-print-hostname ;Boolean | Symbol
> -                    (default #t))
> -  (nice             mingetty-configuration-nice ;Integer | #f
> -                    (default #f))
> -  (chdir            mingetty-configuration-chdir ;String | #f
> -                    (default #f))
> -  (chroot           mingetty-configuration-chroot ;String | #f
> -                    (default #f)))
> +  (mingetty             mingetty-configuration-mingetty ;file-like
> +                        (default mingetty))
> +  (tty                  mingetty-configuration-tty)       ;string
> +  (auto-login           mingetty-configuration-auto-login ;string | #f
> +                        (default #f))
> +  (login-program        mingetty-configuration-login-program ;gexp
> +                        (default #f))
> +  (login-pause?         mingetty-configuration-login-pause? ;Boolean
> +                        (default #f))
> +  (clear-on-logout?     mingetty-configuration-clear-on-logout? ;Boolean
> +                        (default #t))
> +  (delay                mingetty-configuration-delay ;Integer | #f
> +                        (default #f))
> +  (print-issue          mingetty-configuration-print-issue ;Boolean | Symbol
> +                        (default #t))
> +  (print-hostname       mingetty-configuration-print-hostname ;Boolean | Symbol
> +                        (default #t))
> +  (nice                 mingetty-configuration-nice ;Integer | #f
> +                        (default #f))
> +  (chdir                mingetty-configuration-chdir ;String | #f
> +                        (default #f))
> +  (chroot               mingetty-configuration-chroot ;String | #f
> +                        (default #f))
> +  (shepherd-requirement mingetty-configuration-shepherd-requirement
> +                        ;; Since the login prompt shows the host name, wait
> +                        ;; for the 'host-name' service to be done.  Also wait
> +                        ;; for udev essentially so that the tty text is not
> +                        ;; lost in the middle of kernel messages (XXX).
> +                        (default '( user-processes host-name udev
> +                                    virtual-terminal))))
> --8<---------------cut here---------------end--------------->8---
> 
> But it could have look like this:
> 
> --8<---------------cut here---------------start------------->8---
> @@ -1268,23 +1269,27 @@ (define-record-type* <mingetty-configuration>
>    (working-directory mingetty-configuration-working-directory ;String | #f
>                       (default #f))
>    (root-directory    mingetty-configuration-root-directory ;String | #f
> -                     (default #f)))
> +                     (default #f))
> +  (shepherd-requirement mingetty-configuration-shepherd-requirement
> +                        ;; Since the login prompt shows the host name, wait
> +                        ;; for the 'host-name' service to be done.  Also wait
> +                        ;; for udev essentially so that the tty text is not
> +                        ;; lost in the middle of kernel messages (XXX).
> +                        (default '( user-processes host-name udev
> +                                    virtual-terminal))))
> --8<---------------cut here---------------end--------------->8---
> 
> I would like to hear what people who review lot of patches think about
> this.  Does it make your life worse?  Does it not matter?  Or does it
> even help you in some way?
> 
> Thank you for your time and have a nice day,
> Tomas

I agree it makes for a more bloated diff, but it does make it easier to
read when the indentation doesn't change between each field.  What if it
were formatted like this?  No gratuitous white space changes for
formatting and more room for code and comments since it's not smushed on
the right side of the screen.

--8<---------------cut here---------------start------------->8---
@@ -1268,23 +1269,28 @@ (define-record-type* <mingetty-configuration>
   (working-directory mingetty-configuration-working-directory ;String | #f
                      (default #f))
   (root-directory    mingetty-configuration-root-directory ;String | #f
-                     (default #f)))
+                     (default #f))
+  (shepherd-requirement
+    mingetty-configuration-shepherd-requirement
+    ;; Since the login prompt shows the host name, wait
+    ;; for the 'host-name' service to be done.  Also wait
+    ;; for udev essentially so that the tty text is not
+    ;; lost in the middle of kernel messages (XXX).
+    (default '(user-processes host-name udev
+               virtual-terminal))))
--8<---------------cut here---------------end--------------->8---


-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]