unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
To: Jaft <jaft.r@outlook.com>
Cc: 52113@debbugs.gnu.org
Subject: [bug#52113] [PATCH] gnu: Add pnmixer
Date: Sun, 05 Dec 2021 21:43:20 +0100	[thread overview]
Message-ID: <87v902ail3.fsf@nicolasgoaziou.fr> (raw)
In-Reply-To: <DM6PR19MB24607D4AC755449D3F4C3AC799629@DM6PR19MB2460.namprd19.prod.outlook.com> (Jaft's message of "Thu, 25 Nov 2021 19:48:08 +0000 (UTC)")

Hello,

Jaft <jaft.r@outlook.com> writes:

> * gnu/packages/gtk.scm (pnmixer):Add PNMixer

Thank you. Some comments follow.

> +(define-public pnmixer
> +  (let ([version "0.7.2"])

Nitpick: we don't use square brackets for let. Besides, you do not need
a binding here, you just need to hard-code it in the version field.

> +    (package
> +      (name "pnmixer")
> +      (version version)
> +      (source (origin

Could you move origin below source?

> +                (method git-fetch)
> +                (uri (git-reference
> +                      (url "https://github.com/nicklan/pnmixer/")
> +                      (commit (string-append "v" version))))
> +                (file-name (git-file-name name version))
> +                (sha256 (base32
> +                         "0416pa933ddf4b7ph9zxhk5jppkk7ppcq1aqph6xsrfnka4yb148"))

Could you move base32 below sha256 and put the hash string in front of base32?

> +                (modules '((guix build utils)))))

The modules part is not required. You can remove it.

> +    (build-system cmake-build-system)
> +    (arguments `(#:phases (modify-phases %standard-phases (delete 'check))))

The correct way to do this is to add a "#:tests? #f" argument, with
a comment explaining why you are disabling tests.

> +    (native-inputs `(("pkg-config" ,pkg-config)
> +                     ("gettext" ,gettext-minimal)))

Native inputs should be ordered alphabetically. Besides, the list should
be moved on the line below native-inputs.

> +    (inputs `(("alsa-lib" ,alsa-lib)
> +              ("glib" ,glib)
> +              ("libx11" ,libx11)
> +              ("gtk+" ,gtk+)
> +              ("libnotify" ,libnotify)))

Ditto: please order inputs alphabetically and move them below the inputs
field name.

> +    (home-page "https://github.com/nicklan/pnmixer/")
> +    (synopsis "Simple mixer application designed to run in your system tray")

Nitpick: you can remove "your" in the synopsis

Could you send an updated patch?

Regards,
-- 
Nicolas Goaziou

  reply	other threads:[~2021-12-05 20:44 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <529632162.5471310.1637869688182.ref@mail.yahoo.com>
2021-11-25 19:48 ` [bug#52113] [PATCH] gnu: Add pnmixer Jaft
2021-12-05 20:43   ` Nicolas Goaziou [this message]
2021-12-16 21:40     ` Jaft
2021-12-17  8:06       ` bug#52113: " Nicolas Goaziou
2021-12-17 10:34         ` [bug#52113] " Jaft

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://guix.gnu.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87v902ail3.fsf@nicolasgoaziou.fr \
    --to=mail@nicolasgoaziou.fr \
    --cc=52113@debbugs.gnu.org \
    --cc=jaft.r@outlook.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).