Hi Bruno,

Thanks for the review!

On Fri, Jan 13, 2023 at 5:46 PM Bruno Victal <mirai@makinata.eu> wrote:
>
> Hi,
>
> --8<---------------cut here---------------start------------->8---
> +@defvar {Scheme Variable} joycond-service-type
> +Service type for the joycond service.
> +@end defvar
> --8<---------------cut here---------------end--------------->8---
>
> Should be `@defvar joycond-service-type'.

Oh, okay. Guess I missed that change in convention.  There's lots of
'@defvar {Scheme Variable}' in the manual.

> --8<---------------cut here---------------start------------->8---
> +(define-record-type* <joycond-configuration>
> +  joycond-configuration make-joycond-configuration
> +  joycond-configuration?
> +  (joycond joycond-configuration-joycond (default joycond)))
> --8<---------------cut here---------------end--------------->8---
>
> This could be replaced with define-configuration/no-serialization since
> the only field here is a package / file-like object. (see [1], [2] for examples)
> I'd prefer the field be called 'package' here.

Ahhhh unhygienic macros! Not a fan but I see that this macro is the
preferred thing these days so okay, changed!  I also prefer the field
to be called 'package' so I've changed it! When I looked around at
some other services they used the package name as the field name so I
followed their lead.

> --8<---------------cut here---------------start------------->8---
> +(define (joycond-shepherd-service config)
> +  (let ((joycond (joycond-configuration-joycond config)))
> +    (list (shepherd-service
> +           (documentation "Run joycond.")
> +           (provision '(joycond))
> +           (requirement '(bluetooth))
> +           (start #~(make-forkexec-constructor
> +                     (list #$(file-append joycond "/bin/joycond"))))
> +           (stop #~(make-kill-destructor))))))
> --8<---------------cut here---------------end--------------->8---
>
> You might prefer match-record here but this is okay as well.

If I was destructuring many record fields I'd use it.

Updated patch attached.

Thanks!

- Dave