[bug#66557] [PATCH] home: services: Add goimapnotify service.

[Ricardo Wurmus <rekado@elephly.net>]

Hi Nils,

thank you for this service!  I had been looking for a goimapnotify
service just a few days ago, so this will definitely come in handy.

I don’t have the time for a comprehensive review, but I’ll comment on a
few things that stuck out to me.

> +                                    (password-cmd "pass my-private-email-account")
> +                                    (on-new-mail
> +                                      (file-append mbsync "/bin/mbsync private-account"))

It seems wrong to me to compose a command as a single string.  Usually
we separate the executable from the arguments.  People who want to run
the command in a shell can still do that by using “/bin/sh” “-c”
“command string”.

So I think it would be better to let these fields accept command lists.
FILE-APPEND should only join the package value with the file name of the
executable, but not include any arguments.

I suppose these values end up in the generated configuration file as
plain strings anyway, so perhaps it doesn’t matter much.  In that case
please also use FILE-APPEND to embed a reference to PASS.

> +(define (goimapnotify-serialize-field field-name value)
> +  "This is converted to JSON later, so we don't return a string here"
> +  #~(#$(goimapnotify-format-field field-name) . #$value))

Could this be (cons (goimapnotify-format-field field-name) value)
instead?  I don’t think we need this wrapping and unwrapping with G-exp
syntax.

> +(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)))

This looks a little too convoluted to me.  It’s the IF with the
*UNSPECIFIED* as the second branch (followed by the filter) that doesn’t
strike me as nice.  Perhaps you simplify this with a fold?

In any case, the filter predicate could be (negate unspecified?) instead
of the lambda expression.

> ; See https://gitlab.com/shackra/goimapnotify/-/blob/master/config.go?ref_type=heads#L46-62

Please don’t use “master” here, because it’s a moving target, so the
line anchors will become out of date.  Please use an arbitrary recent
commit instead.  Since this is a line comment please use “;;” instead of
“;” (which is used for comments in the margin).

> +(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))

The indentation for the MATCH clauses is too deep.  There are other
instances of oddly deep indentation, such as the body of
“(define-configuration/no-serialization home-goimapnotify-configuration
…)”.  Are you using Emacs?

Since you’re throwing away the first element you can reduce this to:

  (every (compose goimapnotify-account? second) lst)

I haven’t looked at the documentation much, but the capitalization of
“Stdin” and the spurious changes to seemingly empty lines in existing
documentation stood out to me.  It would be better to undo the changes
to unrelated documentation in the same file.

-- 
Ricardo