GNU bug report logs -
#74801
[PATCH] gnu: home: services: Add home-mpv-service-type.
Previous Next
Reported by: Tomas Volf <~@wolfsden.cz>
Date: Wed, 11 Dec 2024 21:54:02 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Full log
View this message in rfc822 format
Hi,
Tomas Volf <~@wolfsden.cz> writes:
> This commit adds a new service type to generate configuration file for the mpv
> media player.
>
> Originally I attempted to use Guix Records (via define-configuration) for
> this, but ran into the bug #74748, so I had to switch to procedures instead.
> The usage is (hopefully) sufficiently described in the documentation. When
> the bug is resolved, I will update it to use define-configuration instead.
>
> The full list of supported options is documented, however I decided to *not*
> document types and purpose for each individual fields. While I had mostly
> working prototype to extract the documentation from mpv, once I realized it
> would be few 10k of lines added, I decided it is not worth it. It would bloat
> the .texi file (by more than 50%), be hard to maintain and, in my opinion,
> would not provide enough value to justify that. The current version seems
> like sane middle ground.
Wow! I didn't know mpv add so many configurable switches.
[...]
> +Due to the bug #74748, it does not use Guix Records to represent the
> +configuration, but uses keyword arguments to achieve similar result.
> +Example follows:
> +
> +@lisp
> +(service home-mpv-service-type
> + (make-home-mpv-configuration
> + #:global (make-mpv-profile-configuration
> + #:fullscreen #t
> + #:alang '("jpn" "eng"))))
> +@end lisp
This looks reasonable if #74748 can't be resolved (I suspect it may
cause an itch to Ludovic, but their plate is pretty full already I
assume!).
[...]
> +@table @asis
> +@item Flags
> +The value is evaluated for truthfulness. Typically you would use
> +@code{#f} or @code{#t}.
nitpick: I'd leave out the implementation details (the phrase about
truthfulness) and simply mention these expect a boolean value, #t or #f.
[...]
> +Only set fields are outputted to the configuration file. Accessors are
> +provided for every field, returning either their value or a sentinel
> +object @code{%unset}.
This should be %unset-value, which is already defined in (gnu services
configuration).
[...]
> +
> +;;;
> +;;; 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.
> + #~(string-append #$(symbol->string field-name)
> + "="
> + #$(if value "yes" "no")
> + "\n"))
> +(define type/boolean?
> + (const #t))
Does the above really check anything?
(type/boolean? "string")
$25 = #t
(type/boolean? 0)
$26 = #t
Seems like no.
> +(define (serialize-type/integer field-name value)
> + #~(string-append #$(symbol->string field-name)
> + "="
> + #$(number->string value)
> + "\n"))
> +(define (type/integer? n)
> + ;; We assume integer is a signed 32bit number.
> + (and-let* (((integer? n))
> + ((>= n -2147483648))
> + ((<= n 2147483647)))))
I'd use the maths to compute the values, for clarity and ensuring
correctness, e.g. (* -1 (expt 2 (1- 32))) and (1- (expt 2 (1- 32))).
> +
> +(define (serialize-type/integer64 field-name value)
> + #~(string-append #$(symbol->string field-name)
> + "="
> + #$(number->string value)
> + "\n"))
> +(define (type/integer64? n)
> + ;; We assume integer is a signed 64bit number.
> + (and-let* (((integer? n))
> + ((>= n -9223372036854775808))
> + ((<= n 9223372036854775807)))))
Likewise.
> +(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. 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.
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'.
Could you send a v2 with the above taken into consideration?
--
Thanks,
Maxim
This bug report was last modified 23 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.