GNU bug report logs - #66557
[PATCH] home: services: Add goimapnotify service.

Previous Next

Full log

View this message in rfc822 format

From: Nils Landt <nils <at> landt.email>
To: Bruno Victal <mirai <at> makinata.eu>
Cc: 66557 <at> debbugs.gnu.org
Subject: [bug#66557] [PATCH] home: services: Add goimapnotify service.
Date: Tue, 21 Nov 2023 16:25:21 +0100 (CET)

Thank you for the feedback, I'll work on implementing it, hopefully on the weekend. 

I've already added a few comments / questions below though.

> Bruno Victal <mirai <at> makinata.eu> hat am 20.11.2023 18:16 CET ls,
> 
> On 2023-10-15 15:01, Nils Landt wrote:
> > This patch adds a home service for generating goimapnotify JSON
> > configuration files.
> 
> […]
> 
> 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.

These options can not be given as command line options, they need to be in a config file.
Personally I don't use shepherd, so I won't be contributing shepherd services :)

> > +@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”.

I think this a good change, and I'm completely fine making it. I just want to mention that I copied these docstrings directly from the project's readme. But I see no reason why the author would capitalize it.

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

Wouldn't that fail because it expects '(goimapnotify-account goimapnotify-account[...])?

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

I'm using vim. The code is indented with autoindent, as mentioned on https://guix.gnu.org/manual/devel/en/html_node/Vim-and-NeoVim.html . If this doesn't match the expected indentation as implemented in Emacs, I'd be grateful if you could indent this file before merging.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.