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

Previous Next

Full log

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

From: Tomas Volf <~@wolfsden.cz>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 74801 <at> debbugs.gnu.org
Subject: Re: bug#74801: [PATCH] gnu: home: services: Add home-mpv-service-type.
Date: Fri, 02 May 2025 18:40:58 +0200

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:

> 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!)

Well, mpv/integer is basically an integer with additional range check.

--8<---------------cut here---------------start------------->8---
(define (mpv/integer? n)
  ;; We assume integer is a signed 32bit number.
  (and-let* (((integer? n))
             ((>= n (* -1 (expt 2 (1- 32)))))
             ((<= n (1-   (expt 2 (1- 32))))))))
--8<---------------cut here---------------end--------------->8---

On the other hand, for e.g. mpv/boolean there is nothing extra to check,
so it is just a matter of consistency to have all "types" sharing the
same naming pattern.

I have renamed all the type/ to mpv/, so hopefully you will find the new
version better. :)

>>>
>>> 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 :-).

And sorry for the delay from my side. :)  I really appreciate you taking
the time to do the review, and have now sent a v2 hopefully addressing
all your concerns.

Have a nice day,
Tomas

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.

Previous Next

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