Re: [PATCH] gnu: add the rc shell package

[Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de>]

Hi Jeff,

thanks for the contribution!  Below are some comments about your patch,
mostly relating to formatting.

> From 5deadfb23d8235101220310d0c47626c1d4c219f Mon Sep 17 00:00:00 2001
> From: Jeff Mickey <j@codemac.net>
> Date: Thu, 9 Jul 2015 17:39:42 -0700
> Subject: [PATCH] gnu: add the rc shell package
>
> * gnu/packages/rc.scm (rc): Add the rc package definition
>
> This patch adds the rc shell package to guix. It is byron's rc, not plan9 rc -
> and on other distributions 'rc' refers to byron's rc and 'plan9port' or some
> other meta package install the plan9 set of tools which includes rc.
>
> It has a zlib license.

We usually don’t add comments like that to the commit message (instead
they go into the cover email to the mailing list).  Also, when creating
a new file we don’t name the variable that is added, but declare this
file to be new.

I wonder if for this shell we could have a common module called
“shells.scm” where we could merge in “zsh.scm” and possibly other
shells.  Anyway, here would be a commit message that is more in line
with the others:

~~~8<~~~~
gnu: Add the rc shell.

* gnu/packages/rc.scm: New file.
~~~8<~~~~

> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append "https://github.com/rakitzis/rc/tarball/"
> +                                  "c884da53a7c885d46ace2b92de78946855b18e92"))
> +              (sha256
> +               (base32 "05hlnqcxaw08m1xypk733hajwaap5pr354ndmrm86k0flisjk0fw"))))

I see that there are no release tarballs.  When we take an arbitrary
commit we usually add a comment.  Also, we normally use the ‘git-fetch’
method instead of ‘url-fetch’.

> +    (build-system gnu-build-system)
> +    (arguments `(#:configure-flags
> +		 '("--with-edit=gnu")
> +		 #:phases
> +		 (modify-phases %standard-phases
> +		   (add-before 'configure 'autoreconf (lambda _
> +							(zero? (system* "autoreconf" "-vfi")))))
> +		 #:tests? #f))

Please add a comment to explain why the tests are disabled (no “check”
target or failing tests?).  The alignment and length of the lines makes
it hard to read.  How about this instead:

    (arguments
     `(#:tests? #f ;no "check" target
       #:configure-flags '("--with-edit=gnu")
       #:phases
       (modify-phases %standard-phases
         (add-before
          'configure 'autoreconf
          (lambda _ (zero? (system* "autoreconf" "-vfi")))))))

> +    (inputs `(("readline" ,readline)
> +	      ("perl" ,perl)))

This is oddly aligned.

> +    (native-inputs `(("autoconf" ,autoconf)
> +		     ("automake" ,automake)
> +		     ("libtool" ,libtool)
> +		     ("pkg-config" ,pkg-config)))

Same here.

> +    (synopsis "An alternative implementation of the plan 9 rc shell.")
> +    (description
> +     "This is a reimplementation for Unix, by Byron Rakitzis, of
> +the Plan 9 shell. It has a small feature set similar to a traditional Bourne
> +shell, but with a much cleaner and simpler syntax.")

Please use two spaces after a period.

I’m not sure if the description is okay; “much cleaner and simpler
syntax” sounds a little too partial to me.

Note that you can use “guix lint” to check your package definition for
the most common problems.

~~ Ricardo