Hi Ludo, Ludovic Courtès <l...@gnu.org> writes:
> Hi, > > Maxim Cournoyer <maxim.courno...@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. Yep. I had that on mind when I made the change, though I still missed a few occurrences. >> (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. Good catch; I got confused there, although my confusion luckily had no side effect :-). > 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. I wanted match on define-configuration'd fields to be backward-compatible with fields migrated from define-record-type*, so that they such change can be made without worrying breakages. It seems good for consistency that both our define-record-type* and define-configuration fields share a similar internal layout, at least until all the old-fashion (ice-9 match) record matching usages are rewritten to use something like 'match-record'. I initially got tricked by that discrepancy and it took me quite some time hunting down a cryptic backtrace when authoring the new mcron configuration records. > 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)) Would this have worked? --8<---------------cut here---------------start------------->8--- scheme@(gnu services mcron)> (define config (mcron-configuration)) scheme@(gnu services mcron)> (struct-ref 0 config) ice-9/boot-9.scm:1685:16: In procedure raise-exception: In procedure struct-ref: Wrong type argument in position 1 (expecting struct): 0 Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue. scheme@(gnu services mcron) [1]> ,q scheme@(gnu services mcron)> ,use (oop goops) scheme@(gnu services mcron)> (class-of config) $5 = #<<class> <<mcron-configuration>> 7fe20fd83e00> --8<---------------cut here---------------end--------------->8--- All in all, I think that's a rather small change that got our internal implementation of both type of records in sync between define-configuration and define-record-type*, that should pave the way for migrating more of the later to the former without risking breaking something, going forward. Alternatively, if we have a good reason to not to go with this, I think we should abstract all the internal-implementation dependent code from our code base (e.g., the match patterns directly matching on field slots). What do you think? -- Thanks, Maxim
