Hi Nils,

On 2023-10-15 15:01, Nils Landt wrote:
> This patch adds a home service for generating goimapnotify JSON
> configuration files.

[…]

> +@lisp
> +(simple-service 'mail-imapnotify-config-examples
> +                home-goimapnotify-service-type
> +                (home-goimapnotify-configuration
> +                  (accounts (list
> +                              `(".config/goimapnotify/private-account.conf"
> +                                ,(goimapnotify-account
> +                                    (host "imap.example.org")
> +                                    (port 993)
> +                                    (tls #t)
> +                                    (username "example")
> +                                    (password-cmd "pass my-private-email-account")
> +                                    (on-new-mail
> +                                      (file-append mbsync "/bin/mbsync private-account"))
> +                                    (on-new-mail-post
> +                                      (file-append mu "/bin/mu index"))
> +                                    (boxes '("INBOX"))))
> +                              `(".config/goimapnotify/work-account.conf"
> +                                ,(goimapnotify-account
> +                                    (host "imap.work.example.org")
> +                                    (port 993)
> +                                    (tls #t)
> +                                    (username "example")
> +                                    (password "12345")
> +                                    (on-new-mail
> +                                      (file-append mbsync "/bin/mbsync work-account"))
> +                                    (on-new-mail-post
> +                                      "notify-send 'New mail'")
> +                                    (boxes '("INBOX"
> +                                            "On Call")))))))))
> +@end lisp
> +
> +Note: to utilize the config files, you need to start a separate goimapnotify
> +process for each one.  Continuing the example above:
> +@code{goimapnotify -conf "$HOME/.config/goimapnotify/private-account.conf"} and
> +@code{goimapnotify -conf "$HOME/.config/goimapnotify/work-account.conf"}.

Not a goimapnotify user but this looks like a daemon application.
I don't like this design much, I think goimapnotify should be launched and managed
using shepherd instead of simply exposing these files to the user.

> +@item @code{tls} (type: maybe-boolean)
> +
> +Use TLS?

Boolean fields are generally suffixed with '?'. (e.g. tls?)

> +@item @code{tls-options} (type: maybe-goimapnotify-tls-options)
> +Option(s) for the TLS connection.  Currently, only one option is
> +supported.
> +
> +@item @code{username} (type: maybe-string)
> +Username for authentication.
> +
> +@item @code{username-cmd} (type: maybe-string-or-file-like)
> +An executable or script that retrieves your username from
> +somewhere, we cannot pass arguments to this command from Stdin.

I'd prefer to write it as “stdin” (lowercase) or expand it to “standard input”.

> +@item @code{xoauth2}
> +(type: maybe-boolean)
> +You can also use xoauth2 instead of password based authentication by
> +setting the xoauth2 option to true and the output of a tool which can
> +provide xoauth2 encoded tokens in passwordCmd.  Examples:
> +@uref{https://github.com/google/oauth2l,Google oauth2l} or
> +@uref{https://github.com/harishkrupo/oauth2ms,xoauth2 fetcher for O36
> +5}.

Same remark as 'tls' option.

> +@item @code{on-new-mail} (type: maybe-string-or-file-like)
> +An executable or script to run when new mail has arrived.
> +
> +@item @code{on-new-mail-post} (type: maybe-string-or-file-like)
> +An executable or script to run after onNewMail has ran.

“An executable or script to run after @code{on-new-mail} has ran.”

> +@table @asis
> +@item @code{reject-unauthorized} (type: maybe-boolean)
> +Skip verifying CA server identify?

Same remark as 'tls' option.

>  @cindex msmtp
>  @uref{https://marlam.de/msmtp, MSMTP} is a @acronym{SMTP, Simple Mail
>  Transfer Protocol} client.  It sends mail to a predefined SMTP server
>  that takes care of proper delivery.
> -
> +
>  The service reference is given below.
> -
> +
>  @defvar home-msmtp-service-type
>  This is the service type for @command{msmtp}.  Its value must be a
>  @code{home-msmtp-configuration}, as shown below.  It provides the
>  @file{~/.config/msmtp/config} file.
> -
> +
>  As an example, here is how you would configure @code{msmtp} for a single
>  account:
> -
> +
>  @lisp
>  (service home-msmtp-service-type
>           (home-msmtp-configuration
> @@ -44739,101 +44876,101 @@ Mail Home Services
>  @end defvar
> 
>  @c %start of fragment
> -
> +
>  @deftp {Data Type} home-msmtp-configuration
>  Available @code{home-msmtp-configuration} fields are:
> -
> +
>  @table @asis
>  @item @code{defaults} (type: msmtp-configuration)
>  The configuration that will be set as default for all accounts.
> -
> +
>  @item @code{accounts} (default: @code{'()}) (type: list-of-msmtp-accounts)
>  A list of @code{msmtp-account} records which contain information about
>  all your accounts.
> -
> +
>  @item @code{default-account} (type: maybe-string)
>  Set the default account.
> -
> +
>  @item @code{extra-content} (default: @code{""}) (type: string)
>  Extra content appended as-is to the configuration file.  Run
>  @command{man msmtp} for more information about the configuration file
>  format.
> -
> +
>  @end table
> -
> +
>  @end deftp
> -
> +
>  @c %end of fragment
> -
> +
>  @c %start of fragment
> -
> +
>  @deftp {Data Type} msmtp-account
>  Available @code{msmtp-account} fields are:
> -
> +
>  @table @asis
>  @item @code{name} (type: string)
>  The unique name of the account.
> -
> +
>  @item @code{configuration} (type: msmtp-configuration)
>  The configuration for this given account.
> -
> +
>  @end table
> -
> +
>  @end deftp
> -
> +
>  @c %end of fragment
> 
>  @c %start of fragment
> -
> +
>  @deftp {Data Type} msmtp-configuration
>  Available @code{msmtp-configuration} fields are:
> -
> +
>  @table @asis
>  @item @code{auth?} (type: maybe-boolean)
>  Enable or disable authentication.
> -
> +
>  @item @code{tls?} (type: maybe-boolean)
>  Enable or disable TLS (also known as SSL) for secured connections.
> -
> +
>  @item @code{tls-starttls?} (type: maybe-boolean)
>  Choose the TLS variant: start TLS from within the session (‘on’,
>  default), or tunnel the session through TLS (‘off’).
> -
> +
>  @item @code{tls-trust-file} (type: maybe-string)
>  Activate server certificate verification using a list of trusted
>  Certification Authorities (CAs).
> -
> +
>  @item @code{log-file} (type: maybe-string)
>  Enable logging to the specified file.  An empty argument disables
>  logging.  The file name ‘-’ directs the log information to standard
>  output.
> -
> +
>  @item @code{host} (type: maybe-string)
>  The SMTP server to send the mail to.
> -
> +
>  @item @code{port} (type: maybe-integer)
>  The port that the SMTP server listens on.  The default is 25 ("smtp"),
>  unless TLS without STARTTLS is used, in which case it is 465 ("smtps").
> -
> +
>  @item @code{user} (type: maybe-string)
>  Set the user name for authentication.
> -
> +
>  @item @code{from} (type: maybe-string)
>  Set the envelope-from address.
> -
> +
>  @item @code{password-eval} (type: maybe-string)
>  Set the password for authentication to the output (stdout) of the
>  command cmd.
> -
> +
>  @item @code{extra-content} (default: @code{""}) (type: string)
>  Extra content appended as-is to the configuration block.  Run
>  @command{man msmtp} for more information about the configuration file
>  format.
> -
> +
>  @end table
> -
> +
>  @end deftp
> -
> +
>  @c %end of fragment

These are unrelated changes, can you drop these hunks?

> +(define (goimapnotify-maybe-serialize field-name value serialization-function)
> +  (if (maybe-value-set? value)
> +    (serialization-function field-name value)
> +    ""))
> +
> +(define (goimapnotify-serialize-maybe-string-or-file-like field-name value)
> + (goimapnotify-maybe-serialize field-name value
> +                               goimapnotify-serialize-string-or-file-like))
> +
> +(define goimapnotify-serialize-string goimapnotify-serialize-field)
> +(define (goimapnotify-serialize-maybe-string field-name value)
> + (goimapnotify-maybe-serialize field-name value goimapnotify-serialize-string))
> +
> +(define (goimapnotify-serialize-maybe-integer field-name value)
> + (goimapnotify-maybe-serialize field-name value goimapnotify-serialize-integer))
> +(define goimapnotify-serialize-integer goimapnotify-serialize-field)
> +
> +(define (goimapnotify-serialize-maybe-boolean field-name value)
> +  (goimapnotify-maybe-serialize field-name value goimapnotify-serialize-boolean))
> +(define goimapnotify-serialize-boolean goimapnotify-serialize-field)
> +
> +(define (goimapnotify-serialize-maybe-list-of-strings field-name value)
> +  (goimapnotify-maybe-serialize field-name value goimapnotify-serialize-list-of-strings))
> +(define (goimapnotify-serialize-list-of-strings field-name value)
> +  (goimapnotify-serialize-field field-name (list->array 1 value)))
> +
> +(define (goimapnotify-serialize-maybe-goimapnotify-tls-options field-name config)
> +  (goimapnotify-maybe-serialize field-name config
> +                                goimapnotify-serialize-goimapnotify-tls-options))

This isn't needed, fields whose maybe-values are unset don't call the
serializing procedures.

> +(define (goimapnotify-serialize-goimapnotify-tls-options field-name config)
> +  (goimapnotify-serialize-field
> +    field-name
> +    (prepare-configuration-for-json config goimapnotify-tls-options-fields)))
> +
> +(define (prepare-configuration-for-json config fields)
> +  "Convert the configuration to the format expected by guile-json.
> +  Unset maybe-values do not appear in the configuration file."
> +  (filter
> +    (lambda (val)
> +      (not (unspecified? val)))
> +    (map
> +      (lambda (field)
> +        (let ((value ((configuration-field-getter field) config)))
> +          (if (maybe-value-set? value)
> +            ((configuration-field-serializer field)
> +             (configuration-field-name field)
> +             value)
> +            *unspecified*)))
> +      fields)))

You can use 'serialize-configuration' instead which accounts for the unset maybe-values.

> +
> +(define-configuration goimapnotify-tls-options
> +                      (reject-unauthorized
> +                        (maybe-boolean)
> +                        "Skip verifying CA server identify?")
> +                      (prefix goimapnotify-))
> +
> +(define-maybe goimapnotify-tls-options)
> +
> +; See https://gitlab.com/shackra/goimapnotify/-/blob/master/config.go?ref_type=heads#L46-62
> +(define-configuration goimapnotify-account
> +                      (host
> +                        (maybe-string)
> +                        "Address of the IMAP server to connect to.")
> +                      (host-cmd
> +                        (maybe-string-or-file-like)
> +                        "An executable or script that retrieves your host from somewhere,
> +                        we cannot pass arguments to this command from Stdin.")
> +                      (port
> +                        (maybe-integer)
> +                        "Port of the IMAP server to connect to.")
> +                      (tls
> +                        (maybe-boolean)
> +                        "Use TLS?")
> +                      (tls-options
> +                        (maybe-goimapnotify-tls-options)
> +                        "Option(s) for the TLS connection. Currently, only one option is
> +                        supported.")
> +                      (username
> +                        (maybe-string)
> +                        "Username for authentication.")
> +                      (username-cmd
> +                        (maybe-string-or-file-like)
> +                        "An executable or script that retrieves your username from
> +                        somewhere, we cannot pass arguments to this command from Stdin.")
> +                      (password
> +                        (maybe-string)
> +                        "Password for authentication.")
> +                      (password-cmd
> +                        (maybe-string-or-file-like)
> +                        "An executable or script that retrieves your password from
> +                        somewhere, we cannot pass arguments to this command from Stdin.")
> +                      (xoauth2
> +                        (maybe-boolean)
> +                        "You can also use xoauth2 instead of password based authentication
> +                        by setting the xoauth2 option to true and the output of a tool
> +                        which can provide xoauth2 encoded tokens in passwordCmd.
> +                        Examples: @url{https://github.com/google/oauth2l, Google oauth2l}
> +                        or
> +                        @url{https://github.com/harishkrupo/oauth2ms, xoauth2 fetcher for O365}.")
> +                      (on-new-mail
> +                        (maybe-string-or-file-like)
> +                        "An executable or script to run when new mail has arrived.")
> +                      (on-new-mail-post
> +                        (maybe-string-or-file-like)
> +                        "An executable or script to run after onNewMail has ran.")
> +                      (wait
> +                        (maybe-integer)
> +                        "The delay in seconds before the mail syncing is triggered.")
> +                      (boxes
> +                        (maybe-list-of-strings)
> +                        "Mailboxes to watch.")
> +                      (prefix goimapnotify-))

You can omit the parentheses around the field-type.

> +(define (list-of-goimapnotify-accounts? lst)
> +  "List is in the form of '((file-name file-like))"
> +  (every (lambda (element)
> +           (match element
> +                  ((string ($ <goimapnotify-account>))
> +                   #t)
> +                  (_ #f)))
> +         lst))

You can replace this with:
--8<---------------cut here---------------start------------->8---
(define list-of-goimapnotify-accounts?
  (list-of goimapnotify-account?))
--8<---------------cut here---------------end--------------->8---

> +(define-configuration/no-serialization home-goimapnotify-configuration
> +                                       (accounts
> +                                         (list-of-goimapnotify-accounts '())
> +                                         "List of accounts that goimapnotify should watch.
> +                                         For each account, a separate configuration file
> +                                         will be generated."))
> +
> +(define (home-goimapnotify-extension old-config extensions)
> +  (match-record old-config <home-goimapnotify-configuration>
> +                (accounts)
> +                (home-goimapnotify-configuration
> +                  (inherit old-config)
> +                  (accounts (append accounts
> +                                    (append-map
> +                                      home-goimapnotify-configuration-accounts
> +                                      extensions))))))

This looks misindented, the .dir-locals file automatically handles
this if you are using emacs.

> +
> +(define home-goimapnotify-service-type
> +  (service-type (name 'home-goimapnotify-service)
> +                (extensions
> +                  (list (service-extension
> +                          home-files-service-type
> +                          goimapnotify-files)))
> +                (compose identity)
> +                (extend home-goimapnotify-extension)
> +                (default-value (home-goimapnotify-configuration))
> +                (description "Configure goimapnotify to execute scripts on IMAP
> +                             mailbox changes.")))

Stylistically, I'd indent this as:

--8<---------------cut here---------------start------------->8---
(define home-goimapnotify-service-type
  (service-type
   (name 'home-goimapnotify-service)
   (extensions …
--8<---------------cut here---------------end--------------->8---


My 2¢!

-- 
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.