Re: Layout of ‘define-configuration’ records

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>>> +               (%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.
>
> [...]
>
>> 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.
>
> That had the opposite effect: it introduced breakage precisely because
> existing uses of ‘match’ on records need to be verified manually, one by
> one.

Yes.  C.f. "I missed a few occurrences" above :-).

> That led me to improve ‘match-record’, to recommend it in the manual,
> and do “convert” some uses of ‘match’ to ‘match-record’:
>
>   https://issues.guix.gnu.org/59390
>
> That’s a good outcome, but I’d prefer not feeling this kind of pressure.

Neat (not the under pressure part)!  Perhaps srfi-worthy?

>> 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.
>
> I see.  However, this is a wide-ranging change, so I think this should
> have been discussed separately from the mcron changes.  I find it
> important to take time for review and discussion for such changes.
>
>>> One last thing: placing ‘%location’ first can let us implement:
>>>
>>>   (define (configuration-location config)
>>>     (struct-ref config 0))
>>
>> Would this have worked?
>>
>> scheme@(gnu services mcron)> (define config (mcron-configuration))
>> scheme@(gnu services mcron)> (struct-ref 0 config)
>
> You got the order wrong.  :-)

Ah!  Thanks for pointing my silly mistake.  Then the argument would
become... if it's good for define-configuration, it should have been
good for define-record-type* the same (why the discrepancy?).

After your new documentation in place to guide users to DTRT with
regards to matching records, if you think %location should be the first
field, then we should make it so in both instances, perhaps?

>> 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.
>
> Fundamentally, the layout of record types should not be visible to
> users.  That it is visible via ‘match’ is the problem, and the solution
> is not to tweak record type layout but instead tp make sure ‘match’ uses
> on records vanish.
>
> I hope that makes sense!

Yes, and I agree.

>> scheme@(gnu services mcron)> ,use (oop goops)
>
> Speaking of which: there was a conscious decision to not use GOOPS in
> Guix from day one.  Perhaps some day we’ll want to collectively question
> that, but let’s make sure we don’t add that dependency on a whim.
>
> For example:
>
>   class-of -> struct-vtable
>   class-name -> record-type-name
>
> See commit 50c17ddd9e2983d71c125d89b422fd20fca476e1 for an example.

Oops!  Another point to add to our future coding style guidelines :-).
Thanks for showing the simple workaround to goops.

-- 
Thanks,
Maxim