Re: [PATCH] gnu/package/pv.scm (pv): New file, new variable

[韋嘉誠]

On Tue, Jun 23, 2015 at 1:43 PM, Alex Kost <address@hidden> wrote:
> Claes Wallin (韋嘉誠) (2015-06-19 21:51 +0300) wrote:

> I think there is no need in this ↑ line.  The commit message should be:
>
> --8<---------------cut here---------------start------------->8---
> gnu: Add pv.
>
> * gnu/package/pv.scm (pv): New file.
> * gnu-system.am (GNU_SYSTEM_MODULES): Add it.
> --8<---------------cut here---------------end--------------->8---

Ok!


> And you also need to add "gnu/package/pv.scm" to "gnu-system.am".  See
> commit 741115b for example.

I will take a look.


>> +;;; Copyright  2012, 2013, 2015 Ludovic Courts <address@hidden>
>> +;;; Copyright  2015 Claes Wallin <address@hidden>
>
> IIUC you are the only author of this file, right?  Then there is no
> place for Ludovic there.

Ok, I copied it from another file and modified it, but if the
convention is that the boilerplate is too thin to be credited, I'll
remove him.

>> +  #:export (pv))
>> +
>> +(define pv
>
> I think we prefer 'define-public' over exporting the package variables,
> but it is probably not a strong convention.

define-public is less redundant. I like it.


> I would put "Pipe Viewer" in parentheses:
>
>         "pv (Pipe Viewer) is a terminal-based tool for monitoring the progress
>
> but I think you may ignore this comment.

No, it's a fair point.


>> +of data through a pipeline. It can be inserted into any normal pipeline
>    of data through a pipeline.  It can be inserted into any normal pipeline
>
> I realize that you took this description from the home page, but our
> convention is to use two spaces between sentences.

If there's a convention I'll follow it. I'll just note here once that
I disagree with it. Double space is a type-writer convention that
"nobody" follows any more. ;-)

Regarding scraping the text I tried quickly to see what license the
web site text is under, but didn't find anything conclusive. I figured
if Debian is copying it, I should be fine. Maybe I'll get back to
checking it later.


Thanks for the feedback. I have several more packages in the pipeline
once I get this one right!

--
   /c