guix-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] gnu: Add beep.


From: José Miguel Sánchez García
Subject: Re: [PATCH] gnu: Add beep.
Date: Wed, 04 Jan 2017 23:32:28 +0100
User-agent: Roundcube Webmail/1.0.6

On 2017-01-04 08:10, Tobias Geerinckx-Rice wrote:
José,

On 02/01/17 20:33, José Miguel Sánchez García wrote:
Add beep command.

Thanks!

-;;; Copyright © 2016 José Miguel Sánchez García...
+;;; Copyright © 2017 José Miguel Sánchez García...

Please keep both years:

  ;;; Copyright © 2016, 2017 José Miguel Sánchez García...

(base32 "0bgch6jq5cahakk3kbr9549iysf2dik09afixxy5brbxk1xfzb2r"))))

A bit long, easily solved by moving the hash string to its own line:

  (base32
   "0bgch6jq5cahakk3kbr9549iysf2dik09afixxy5brbxk1xfzb2r"))))

; It doesn't install without this

I'd consider that implied, and just drop this comment.

(Future note: since in-line comments like this one aren't considered
full sentences, they don't start with a capital letter. Full-line
comments beginning with ;; are, and do.)

You're also doing more than patching the makefile here. Consider moving
the ‘mkdir-p’ part to a separate, later phase (add-before 'install).
Sorry to ruin your ‘let’ factoring...

(synopsis "Command line utility for Linux that beeps the PC speaker.")
(description "Command line utility for Linux that beeps the PC
              speaker.")

I'll trust you that this is indeed Linux-specific: the manual mentions
ioctls, and I can't get it to make a sound from within X :-(

s/command line/command-line/ when used as an adjective.

Descriptions should be between 5 and 10 lines where possible, use full
sentences, and not repeat the synopsis ($ info guix "Synopses and
Descriptions").

Luckily, the first paragraph of the man page makes a pretty good
starting point:

  beep allows the user to control the PC speaker with precision,
  allowing different sounds to indicate different events.  While it can
  be run quite happily on the command line, it's intended place of
  residence is within shell/perl scripts, notifying the user when
  something interesting occurs.  Of course, it has  no notion of what's
  interesting, but it's real good at that notifying part.

I'd re-write the synopsis as ‘Linux command-line utility to control the
PC speaker’. It does a tad more than just beep, even if it's the musical
equivalent of burping the alphabet.

(home-page "http://www.johnath.com/beep";)
                                          ^^^

Your package is missing a few brackets. Did the patch get truncated?

Kind regards,

T G-R
I've applied your changes, ran guix lint and tested it. Here you have the fixed patch.

--
José Miguel Sánchez García

Attachment: 0001-gnu-Add-beep.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]