Subject: Re: [bug#37738] [PATCH] gnu: add lxqt-connman-applet

From: Ludovic Courtès <[email-masked]>
Date: Fri, 18 Oct 2019 11:45:13 +0200

Hi Reza,

"Reza Alizadeh Majd" <[email-masked]> skribis:

> From c3756518da1289ec7f7cc803e624632aa21aed01 Mon Sep 17 00:00:00 2001
> From: Reza Alizadeh Majd <[email-masked]>
> Date: Sun, 13 Oct 2019 20:55:21 +0330
> Subject: [PATCH] gnu: add lxqt-connman-applet
>
> * gnu/packages/lxqt.scm (lxqt-connman-applet) New variable.

Nice.  A few comments/questions:

> +(define-public lxqt-connman-applet
> +  (let ((commit "3db374eebd8d851f68a50fc5d1ef5fa9478c275e")
> +        (revision "0"))

Why did you choose this specific commit?

Normally we only package upstream releases (tags); we resort to commits
in between releases only when there’s a good reason to do that, and in
that cases it’s better to add a comment explaining why we did that.

> +      (arguments
> +        `(#:tests? #f

Could you add a comment explaining why tests are disabled?

> +      (home-page "https://github.com/lxqt/lxqt-connman-applet")
> +      (synopsis "System-tray applet for connman")
> +      (description "LXQt based system-tray applet for connman")

Please write a full description:

  https://guix.gnu.org/manual/en/html_node/Synopses-and-Descriptions.html

Apart from that the patch LGTM.

Could you send an updated patch?

Thank you!

Ludo’.