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
