Subject: Re: [bug#75959] [PATCH v10] services: syncthing: Add support for config file generation.

From: Ludovic Courtès <[email-masked]>
Date: Mon, 17 Feb 2025 11:26:00 +0100

Hi,

Zacchaeus Scheffer <[email-masked]> skribis:

>>>From a573fd78e6b8d10b32eb10a753423073c7bbaeef Mon Sep 17 00:00:00 2001
> From: Zacchaeus <[email-masked]>
> Date: Sun, 21 Jul 2024 00:54:25 -0700
> Subject: [PATCH v10] services: syncthing: Add support for config file
>  generation.
>
> * gnu/services/syncthing.scm: (syncthing-config-file,
> syncthing-folder, syncthing-device, syncthing-folder-device): New
> records;  (syncthing-service-type): Add special-files-service-type
> extension for the config file; (syncthing-files-service): Add service
> to create config file.
> * gnu/home/services/syncthing.scm: (home-syncthing-service-type):
> Extend home-files-services-type and re-exported more things from
> gnu/services/syncthing.scm.
> * doc/guix.texi: (syncthing-service-type): Document changes.
>
> Change-Id: I87eeba1ee1fdada8f29c2ee881fbc6bc4113dde9

This is a nice improvement!  Much better than fiddling with the GUI.
:-)

Sorry for not chiming in earlier; I just noticed a couple of stylistic
issues:

> +This section documents a subset of the Syncthing configuration
> +options—specifically those related to Guix or those affecting how your
> +computer will connect to other computers over the network (such as
> +Syncthing relays or discovery servers).  The configuration is fully
> +documented in the upstream
> +@uref{https://docs.syncthing.net/users/config.html, Syncthing config
> +documentation}; camelCase there is converted to kebab-case here.  If you

“Kebab-case”, really? :-)

> +@table @asis
> +@item @code{folders} (default: @var{(list (syncthing-folder (id "default") (label "Default Folder") (path "~/Sync")))}

Should be @code, not @var (throughout this table).

Could you send a patch fixing this?

BTW, ‘define-configuration’ would make it easier to generate this doc.

> +@item @code{gui-enabled} (default: @var{"true"})

The naming convention used in all of Guix is to add a final question
mark for Boolean values; this would be ‘gui-enabled?’.

As for the value itself, it would help a lot to use #t and #f instead of
the strings "true" and "false" (both of which have truth value in
Scheme).  It leads to a bit of extra work in the serializer, but I think
it’s worth it.  Because then we can also have type-checking of fields.

> +@item @code{gui-tls} (default: @var{"false"})
> +@item @code{gui-debugging} (default: @var{"false"})
> +@item @code{gui-send-basic-auth-prompt} (default: @var{"false"})
> +@item @code{gui-address} (default: @var{"127.0.0.1:8384"})
> +@item @code{gui-user} (default: @var{#f})
> +@item @code{gui-password} (default: @var{#f})
> +A bcrypt hash of the GUI password.  Remember that this will be globally
> +exposed in @file{/gnu/store}.

I believe you want @itemx for all but the first item.

> +@item @code{local-announce-port} (default: @var{"21027"})
> +@item @code{local-announce-mcaddr} (default: @var{"[ff12::8384]:21027"})
> +@item @code{max-send-kbps} (default: @var{"0"})
> +@item @code{max-recv-kbps} (default: @var{"0"})
> +@item @code{reconnection-interval-s} (default: @var{"60"})

Similar to what I wrote above: numbers should be numbers, not strings.

Last note: the convention throughout Guix it to avoid abbreviations.

I’m not sure about the way forward; maybe we can make those typing
changes (and perhaps some naming changes) and afford some
incompatibility because the service is young?  WDYT?

Ludo’.