Re: [PATCH] gnu: Add beep.

[Tobias Geerinckx-Rice <me@tobias.gr>]

[-- Attachment #1.1: Type: text/plain, Size: 2402 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 476 bytes --]