* Regarding the vertical alignment in the record definitions
@ 2024-12-03 0:34 Tomas Volf
2024-12-05 7:13 ` Maxim Cournoyer
2024-12-05 7:35 ` Efraim Flashner
0 siblings, 2 replies; 5+ messages in thread
From: Tomas Volf @ 2024-12-03 0:34 UTC (permalink / raw)
To: guix-devel
[-- Attachment #1: Type: text/plain, Size: 5353 bytes --]
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
--
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: Regarding the vertical alignment in the record definitions
2024-12-03 0:34 Regarding the vertical alignment in the record definitions Tomas Volf
@ 2024-12-05 7:13 ` Maxim Cournoyer
2024-12-05 8:41 ` indieterminacy
2024-12-05 7:35 ` Efraim Flashner
1 sibling, 1 reply; 5+ messages in thread
From: Maxim Cournoyer @ 2024-12-05 7:13 UTC (permalink / raw)
To: guix-devel
Hi Tomas,
Tomas Volf <~@wolfsden.cz> writes:
> Hello Guix,
>
> I would like to bring up for debate the convention the project has of
> vertically aligning the record definitions.
I don't think that's a project convention. It's more like it's a
convention of some individual in the projects :-). At least I don't
follow it, except sometimes when touching a piece of code already
indented that way.
> 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:
I agree it's a bit tedious, both manually and also in diffs. My
personal preference is to leave just one space between the field name
and the value, that also holds for variable bounds in lets, etc., to
avoid the problem (at the cost of some visual clarity, I guess).
One day maybe we'll have a general tool like 'scheme-fmt' to run on a
file save hook that'd fix the format question for good, like Python has
with 'black' or 'ruff', etc. I have on mind to work on such a tool, but
it's low in the list.
--
Thanks,
Maxim
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Regarding the vertical alignment in the record definitions
2024-12-05 7:13 ` Maxim Cournoyer
@ 2024-12-05 8:41 ` indieterminacy
2024-12-09 0:31 ` Maxim Cournoyer
0 siblings, 1 reply; 5+ messages in thread
From: indieterminacy @ 2024-12-05 8:41 UTC (permalink / raw)
To: Maxim Cournoyer; +Cc: guix-devel
Hello,
On 2024-12-05 07:13, Maxim Cournoyer wrote:
> Hi Tomas,
>
> ...
>
> I agree it's a bit tedious, both manually and also in diffs. My
> personal preference is to leave just one space between the field name
> and the value, that also holds for variable bounds in lets, etc., to
> avoid the problem (at the cost of some visual clarity, I guess).
>
> One day maybe we'll have a general tool like 'scheme-fmt' to run on a
> file save hook that'd fix the format question for good, like Python has
> with 'black' or 'ruff', etc. I have on mind to work on such a tool,
> but
> it's low in the list.
FWIW, Im slowly cobbling together some parsing-expression-grammars for
Guix package definitions.
Im doing it in the Lisp TXR atm but Im going to create a hybrid approach
with Prolog to make it more general.
In addition to outputting a unified formatting, Im planning on altering
descriptions so that that that content is outputted in the format
Texinfo.
Please dont expect anything soon but should anybody start any other
initiative then I may possibly some useful assets or details by then.
Kind regards,
Jonathan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Regarding the vertical alignment in the record definitions
2024-12-05 8:41 ` indieterminacy
@ 2024-12-09 0:31 ` Maxim Cournoyer
0 siblings, 0 replies; 5+ messages in thread
From: Maxim Cournoyer @ 2024-12-09 0:31 UTC (permalink / raw)
To: indieterminacy; +Cc: guix-devel
Hello,
indieterminacy <indieterminacy@libre.brussels> writes:
> Hello,
>
> On 2024-12-05 07:13, Maxim Cournoyer wrote:
>> Hi Tomas,
>> ...
>> I agree it's a bit tedious, both manually and also in diffs. My
>> personal preference is to leave just one space between the field name
>> and the value, that also holds for variable bounds in lets, etc., to
>> avoid the problem (at the cost of some visual clarity, I guess).
>> One day maybe we'll have a general tool like 'scheme-fmt' to run on
>> a
>> file save hook that'd fix the format question for good, like Python has
>> with 'black' or 'ruff', etc. I have on mind to work on such a tool,
>> but
>> it's low in the list.
>
> FWIW, Im slowly cobbling together some parsing-expression-grammars for
> Guix package definitions.
> Im doing it in the Lisp TXR atm but Im going to create a hybrid
> approach with Prolog to make it more general.
>
> In addition to outputting a unified formatting, Im planning on
> altering descriptions so that that that content is outputted in the
> format Texinfo.
>
> Please dont expect anything soon but should anybody start any other
> initiative then I may possibly some useful assets or details by then.
Sounds neat/useful!
--
Thanks,
Maxim
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Regarding the vertical alignment in the record definitions
2024-12-03 0:34 Regarding the vertical alignment in the record definitions Tomas Volf
2024-12-05 7:13 ` Maxim Cournoyer
@ 2024-12-05 7:35 ` Efraim Flashner
1 sibling, 0 replies; 5+ messages in thread
From: Efraim Flashner @ 2024-12-05 7:35 UTC (permalink / raw)
To: guix-devel; +Cc: Tomas Volf
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-12-09 0:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-03 0:34 Regarding the vertical alignment in the record definitions Tomas Volf
2024-12-05 7:13 ` Maxim Cournoyer
2024-12-05 8:41 ` indieterminacy
2024-12-09 0:31 ` Maxim Cournoyer
2024-12-05 7:35 ` Efraim Flashner
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).