guix-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Add r-scran 1.2.0 and all its dependencies


From: Ra
Subject: Re: [PATCH] Add r-scran 1.2.0 and all its dependencies
Date: Tue, 27 Dec 2016 16:34:16 +0000

Hi Ricardo,

On Tue, Dec 27, 2016 at 5:02 PM Ricardo Wurmus <address@hidden> wrote:

Hi Raoul,

> in attachment a patch for the inclusion of an  R/Bioconductor library named
> "scran" https://bioconductor.org/packages/release/bioc/html/scran.html ,
> scran has a lot of dependencies and I placed those related to
> bioinformatics/bioconductor into bioinformatics.scm and those related just
> to R insto statistics.scm.

Nice!  Thank you for your efforts!
Thanks, the result worth the effort.
 

> Let me know if this big-patch is acceptable or how do I need to submit it.

We have a policy of one commit per package.  Each commit has a commit
summary in a standard format (you’ll see when running git log).
Ok, when I was adding r-scran I was just trying the make it the work. I'll go back and create a patch for each dependency.


Going through the big patch I noticed a couple of things:

* the indentation is rather uneven.  If you’re using Emacs it’s very
  simple to correct this by using TAB and/or hitting C-M-q on the
  _expression_.  (Also make sure parentheses are not all by themselves on
  an otherwise empty line.)
I was using Vim, I will try with Emacs and also I was practicing a bit.
 

* line length (especially in descriptions) exceeds the limits.

* Please edit the descriptions.  Package descriptions for Guix have to
  use full sentences.

Judging from the indentation this doesn’t look like it was created by
the *recursive* importer.  Were there any problems using the recursive
CRAN/Bioconductor importer?
with guix 0.12.0 I was able to use guix import, I had some problem in the past due to certificates and x509.
I used:

guix import cran --archive=bioconductor scater

I had to add the missing dependencies by hand, is there a recursive way to do so?



The package for “r-shiny” needs a closer look.  AFAIR it contains
minified _javascript_, which does not count as actual source code.  If
that’s in fact the case we would need to fiddle with the package a
little to replace minified _javascript_ blobs with _javascript_ source files
(which may be minified as part of the build process).  I’d prefer if
“r-shiny” were added to “web.scm”.

Ok for web.scm.
 

In the “r-statmod” package _expression_ the comment above the gfortran
input (which should be a native input) doesn’t seem to apply.
Ok, bad comment placed there during tests.
 
Please also double-check that the licenses are correct.  The importer
may not necessary get things right.
Actually I noticed that and I fixed them, I will check again. 

Finally, please use lower-case words in the synopsis.  In R packages
this is often capitalised.
Ok. 

Could you please send an updated patch set?
Sure, in a couple of days, I think.

--
Raoul

reply via email to

[Prev in Thread] Current Thread [Next in Thread]