diff mbox series

[bug#58855,1/5] services: configuration: Re-order generated record fields.

Message ID 20221029041649.12144-1-maxim.cournoyer@gmail.com
State New
Headers show
Series [bug#58855,1/5] services: configuration: Re-order generated record fields. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git-branch success View Git branch
cbaines/applying patch success
cbaines/issue success View issue

Commit Message

Maxim Cournoyer Oct. 29, 2022, 4:16 a.m. UTC 
This is so that the first field of the generated record matches the first one
declared, which makes 'define-configuration' record API compatible with
define-record-type* ones.

* gnu/services/configuration.scm (define-configuration-helper): Move the
%location field below the ones declared by the user.
* gnu/services/monitoring.scm (zabbix-front-end-config): Adjust match pattern
accordingly.
---
 gnu/services/configuration.scm | 10 +++++-----
 gnu/services/monitoring.scm    |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Ludovic Courtès Nov. 17, 2022, 10:37 p.m. UTC | #1 
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> This is so that the first field of the generated record matches the first one
> declared, which makes 'define-configuration' record API compatible with
> define-record-type* ones.
>
> * gnu/services/configuration.scm (define-configuration-helper): Move the
> %location field below the ones declared by the user.
> * gnu/services/monitoring.scm (zabbix-front-end-config): Adjust match pattern
> accordingly.

[...]

> +++ b/gnu/services/configuration.scm
> @@ -242,17 +242,17 @@ (define-record-type* #,(id #'stem #'< #'stem #'>)
>                 stem
>                 #,(id #'stem #'make- #'stem)
>                 #,(id #'stem #'stem #'?)
> -               (%location #,(id #'stem #'stem #'-location)
> -                          (default (and=> (current-source-location)
> -                                          source-properties->location))
> -                          (innate))
>                 #,@(map (lambda (name getter def)
>                           #`(#,name #,getter (default #,def)
>                                     (sanitize
>                                      #,(id #'stem #'validate- #'stem #'- name))))
>                         #'(field ...)
>                         #'(field-getter ...)
> -                       #'(field-default ...)))
> +                       #'(field-default ...))
> +               (%location #,(id #'stem #'stem #'-location)
> +                          (default (and=> (current-source-location)
> +                                          source-properties->location))
> +                          (innate)))

Moving the field last is problematic as we’ve seen for any user that
uses ‘match’ on records—something that’s not recommended but still used
a lot.

>  (define (zabbix-front-end-config config)
>    (match-record config <zabbix-front-end-configuration>
> -    (%location db-host db-port db-name db-user db-password db-secret-file
> -               zabbix-host zabbix-port)
> +    (db-host db-port db-name db-user db-password db-secret-file
> +             zabbix-host zabbix-port %location)

This change has no effect because ‘match-record’ matches fields by name,
precisely to avoid problems when changing the layout of record types.

I’m not sure what was meant by “compatible” in the commit log; how
fields are laid out is something user code should not be aware of.

The only thing to keep in mind is that records must not be matched with
‘match’ because then the code silently breaks when the record type
layout is changed.  This is why ‘match-record’ was introduced:

  https://issues.guix.gnu.org/28960#4

One last thing: placing ‘%location’ first can let us implement:

  (define (configuration-location config)
    (struct-ref config 0))

As if there were type inheritance.  I didn’t see any indication that
this is actually done anywhere, but it could have been the case (it’s
not an unusual pattern) and it’s still something we might want to do.

Does that make sense?

Thanks,
Ludo’.
diff mbox series

Patch

diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
index 636c49ccba..dacfc52ba9 100644
--- a/gnu/services/configuration.scm
+++ b/gnu/services/configuration.scm
@@ -242,17 +242,17 @@  (define-record-type* #,(id #'stem #'< #'stem #'>)
                stem
                #,(id #'stem #'make- #'stem)
                #,(id #'stem #'stem #'?)
-               (%location #,(id #'stem #'stem #'-location)
-                          (default (and=> (current-source-location)
-                                          source-properties->location))
-                          (innate))
                #,@(map (lambda (name getter def)
                          #`(#,name #,getter (default #,def)
                                    (sanitize
                                     #,(id #'stem #'validate- #'stem #'- name))))
                        #'(field ...)
                        #'(field-getter ...)
-                       #'(field-default ...)))
+                       #'(field-default ...))
+               (%location #,(id #'stem #'stem #'-location)
+                          (default (and=> (current-source-location)
+                                          source-properties->location))
+                          (innate)))
 
              (define #,(id #'stem #'stem #'-fields)
                (list (configuration-field
diff --git a/gnu/services/monitoring.scm b/gnu/services/monitoring.scm
index 9c8704092c..b19c6c9f18 100644
--- a/gnu/services/monitoring.scm
+++ b/gnu/services/monitoring.scm
@@ -622,8 +622,8 @@  (define-configuration zabbix-front-end-configuration
 
 (define (zabbix-front-end-config config)
   (match-record config <zabbix-front-end-configuration>
-    (%location db-host db-port db-name db-user db-password db-secret-file
-               zabbix-host zabbix-port)
+    (db-host db-port db-name db-user db-password db-secret-file
+             zabbix-host zabbix-port %location)
     (mixed-text-file "zabbix.conf.php"
                      "\
 <?php