Dear Leo and Ricardo,

Thank you for reviewing this patch.  I hope it is fine now.

Leo Famulari writes:

> On Tue, Mar 08, 2016 at 09:10:14PM +0100, Roel Janssen wrote:
>> From 990eda92a62dc25d0f5792437a82e119c5e3579f Mon Sep 17 00:00:00 2001
>> From: Roel Janssen <roel@gnu.org>
>> Date: Tue, 8 Mar 2016 21:06:53 +0100
>> Subject: [PATCH] gnu: Add bioawk.
>> 
>> * gnu/packages/bioinformatics.scm (bioawk): New variable.
>
> Thanks for the patch!
>
> [...]
>
>> +    (propagated-inputs
>> +     `(("zlib" ,zlib)))
>
> I changed this to a plain input, and then checked the references of the
> resulting build, and zlib is referenced. Considering that, does it need
> to be propagated into the user's profile? Propagated inputs should be
> avoided if possible.

You're right.  I simply get confused about inputs, native-inputs and
propagated-inputs.

I made it an input in my new patch.

>> +    (native-inputs
>> +     `(("bison" ,bison)))
>> +    (arguments
>> +     `(#:parallel-build? #f
>> +       #:phases
>> +       (modify-phases %standard-phases
>> +         (delete 'configure)
>> +         (delete 'check)
>
> Can you say why tests are disabled? It can be as simple as "no test
> suite" if that is accurate.

Sure.

>> +         (replace
>> +          'install
>> +          (lambda* (#:key outputs #:allow-other-keys)
>> +            (let ((bin (string-append (assoc-ref outputs "out") "/bin")))
>> +              (install-file "bioawk" bin)))))))
>
> The git repo includes a manpage 'awk.1'. Can you send an updated patch
> that installs that as well?

I added it as bioawk.1 instead, to avoid confusion with awk.
Good catch!


Ricardo Wurmus writes:

> Roel Janssen <roel@gnu.org> writes:
>
>> Please let me know when something is wrong with the patch.
>
> In addition to Leo’s comments here are mine:
>
>> +    (arguments
>> +     `(#:parallel-build? #f
>
> Why is parallel-build disabled?  Could you add a comment?

Sure.

>> +       #:phases
>> +       (modify-phases %standard-phases
>> +         (delete 'configure)
>> +         (delete 'check)
>
> We usually just do “#:tests? #f” with a comment, instead of deleting the
> “check” phase.

Sorry for the redundancy.  I submitted two patches before receiving
comments about this.  Fixed in my new patch, and I'll apply this in
future patches as well.

>> +         (replace
>> +          'install
>
> Please put “'install” on the same line as “(replace”.

Should I do this for other (replace ...) as well?  If I want to replace
the build phase, should I put it on the same line as well?

>> +          (lambda* (#:key outputs #:allow-other-keys)
>> +            (let ((bin (string-append (assoc-ref outputs "out") "/bin")))
>> +              (install-file "bioawk" bin)))))))
>> +    (home-page "https://github.com/lh3/bioawk")
>> +    (synopsis "AWK with bioinformatics extensions")
>> +    (description "Bioawk is an extension to Brian Kernighan's awk, adding the
>> +support of several common biological data formats, including optionally gzip'ed
>> +BED, GFF, SAM, VCF, FASTA/Q and TAB-delimited formats with column names.  It
>> +also adds a few built-in functions and an command line option to use TAB as the
>
> “a command-line option”, not “an”

Good catch!  Fixed in my new patch.

Kind regards,
Roel Janssen