[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] gnu: add the rc shell package
From: |
Ricardo Wurmus |
Subject: |
Re: [PATCH] gnu: add the rc shell package |
Date: |
Fri, 10 Jul 2015 15:59:30 +0200 |
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 <address@hidden>
> 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
- [PATCH] gnu: add the rc shell package, Jeff Mickey, 2015/07/10
- Re: [PATCH] gnu: add the rc shell package,
Ricardo Wurmus <=
- Re: [PATCH] gnu: add the rc shell package, Mark H Weaver, 2015/07/11
- Re: [PATCH] gnu: add the rc shell package, Jeff Mickey, 2015/07/11
- Re: [PATCH] gnu: add the rc shell package, Mark H Weaver, 2015/07/11
- Re: [PATCH] gnu: add the rc shell package, Jeff Mickey, 2015/07/12
- Re: [PATCH] gnu: add the rc shell package, Jeff Mickey, 2015/07/12
- Re: [PATCH] gnu: add the rc shell package, Mark H Weaver, 2015/07/12
- Re: [PATCH] gnu: add the rc shell package, Jeff Mickey, 2015/07/13
- Re: [PATCH] gnu: add the rc shell package, Mark H Weaver, 2015/07/13