GNU bug report logs - #74801
[PATCH] gnu: home: services: Add home-mpv-service-type.

Previous Next

Full log

Message #17 received at 74801 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Tomas Volf <~@wolfsden.cz>
Cc: 74801 <at> debbugs.gnu.org
Subject: Re: bug#74801: [PATCH] gnu: home: services: Add home-mpv-service-type.
Date: Wed, 23 Apr 2025 14:39:59 +0900

Hi Tomas,

Tomas Volf <~@wolfsden.cz> writes:

[...]

>>> +
>>> +;;;
>>> +;;; Basic types.
>>> +;;;
>>> +(define (serialize-type/boolean field-name value)
>>
>> Nitpick: it's more common in the code base to name serializers as
>> 'serialize-boolean'; it'd be nicer to stick to that naming style, for
>> consistency.
>
> But it does follow the same pattern.  The type is named type/boolean.
> So the pattern of serialize-$TYPE results in serialize-type/boolean.
> Without the type/ prefix I would run into collisions, since I need a
> predicate for an integer (type/integer?), but integer? is already a
> procedure doing something else.  I am not sure I want to shadow it.
>
> Hm, would it make it better for you if I replaced the type/ prefix with
> mpv/ prefix?  So mpv/integer, instead of type/integer?  That would
> result in serialize-mpv/boolean, which might be better in your eyes?

I think that'd be more precise naming yes, if there's a valid reason
that mpv/integer? != integer? (which I'm sure there is since you went to
the trouble of defining it!)

[...]

>>> +(define (serialize-type/string field-name value)
>>> +  #~(string-append #$(symbol->string field-name)
>>> +                   "="
>>> +                   #$value
>>> +                   "\n"))
>>> +(define type/string?
>>> +  string?)
>>> +
>>> +(define (serialize-type/float field-name value)
>>> +  #~(string-append #$(symbol->string field-name)
>>> +                   "="
>>> +                   #$(number->string (exact->inexact value))
>>> +                   "\n"))
>>> +(define type/float?
>>> +  ;; I am not sure how to validate floats.
>>> +  real?)
>>
>> Maybe inexact? would be closer.
>
> However values satisfying integer? should be accepted as well, at least
> from the user point of view it should be fine to write just 2, not 2.0,
> so inexact? does not seem ideal here.

OK!

>> For floats you could check that
>>
>> (type/integer? (inexact->exact (truncate value))) is true.
>>
>> For doubles you could check that
>>
>> (type/integer64? (inexact->exact (truncate value))) is true.
>>
>> I think.
>
> I am not sure this is correct, since floats/doubles can have large range
> than integers.  So, if you do not mind too much, I will leave the real?
> for now.  In practice I believe it should work well enough.

OK!  I don't mind, and I haven't got refreshed on the IEEE
representations of numbers before I commented ;-).

>> For the rest, it looks good to me, except that you throw old fashioned
>> exception values.  Please take a look in the code base to see how
>> raising error message exceptions that get shown nicely by the Guix command
>> line.
>>
>> Maybe define-compile-time-procedure from (guix combinators) can be used,
>> see it used for example in (gnu services base) for
>> `assert-valid-name'.
>
> I was not able to wrap my head around define-compile-time-procedure, but
> I took some other inspiration in the (gnu services base) module and used
> formatted-message.  Seems to be printed well by guix build.
>
> guix build: error: option fullscreena not found for mpv-profile-configuration
>
>
> and
>
> guix build: error: invalid mpv configuration for fullscreen: yes

That's good enough to start!  It can always be refined later.

>>
>> Could you send a v2 with the above taken into consideration?
>
> Once I know what to do with the type/..., will send. :)

Sorry for the delay.  Now you know :-).

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.