macroevolution / bammtools

BAMMtools is an R package for analysis of BAMM results
10 stars 6 forks source link

Devtools #39

Closed josephwb closed 8 years ago

josephwb commented 8 years ago

Ok, this is an admittedly large PR, so some notes.

The idea here is to begin using the tools available in devtools. Eventually, this will include unit testing, but the current PR is almost completely involved with documentation.

The devtools paradigm with regards to documentation is that code and documentation should reside together (i.e. same file). This prevents changes being made out of lockstep. Devtools (via roxygen) extracts the documentation and creates the familiar .Rd files that we all know and love hate. Moreover, it formats things consistently, so all doc pages have the same feel. It also writes the NAMESPACE file, so only functions that need to be exported are exported (compare the present NAMESPACE file with the one in the master branch). No more mucking around with .Rd and NAMESPACE files anymore (there is a warning at the top of each file to explicitly not do this)!

To generate the documentation, all you need to do is (from anywhere within the package directory):

require(devtools);
document();

and it will update any doc files that need it (and not others, like what make does). Devtools will report any documentation problems during this step, so they can be easily fixed. Try it out: delete all documentation files:

rm man/*
rm NAMESPACE

and rebuild (in R):

require(devtools);
document();

Because this is automatically generated, the documentation files will be identical to before (notice that git will not recognize any file edits).

Devtools also allows one to perform the check you would normally do with R CMD check BAMMtools_2.1.2.tar.gz, but without needing to leave R to do it. Simply:

require(devtools);
check();

by default the settings here are the same as what CRAN uses, so it is more stringent than the normal R CMD check BAMMtools_2.1.2.tar.gz.

Anyway, here are the major changes:

  1. Migration of documentaion to .R files, which in turn enables automatic (re)generation of NAMESPACE and .Rd files.
  2. Fixing some doc problems (not comprehensive, but pretty good), including: code formatting, capitalization, fix all bad urls, adding some argument types, etc.
  3. Removing .o files from the repo, and not tracking them. Since BAMMtools needs to be built anyway, there is no sense in distributing .o files (which may be for the wrong system anyway).
  4. Deprecating richColors. This was copied (with no exported documentation) from the gplots package, which is not ideal. Instead, gplots is now a Depends package. For backwards compatibility I've included a pared-down richColors function, which calls rich.colors but also prints a note to the screen that richColors is deprecated.
  5. Increased stringency of the travis builds. The changes here run checks as CRAN does, but even more stringently: it also runs code marked as dontrun (which is not a lot, maybe 2 functions).

Erm, I think that is it. As you can see, the edits are all passing the (more stringent) travis checks.

HTH. :bowtie:

jonchang commented 8 years ago

Great work @josephwb!! This will make hacking on BAMMtools much nicer.

ptitle commented 8 years ago

This sounds excellent, thanks so much for working on this!

I tried testing out the document() function as you suggested, but I get an error. After deleting the man/ directory and the NAMESPACE, I loaded devtools, set the working directory to bammtools/BAMMtools, but I get the following:

> setwd('~/bammtools/BAMMtools')
> list.files()
[1] "data"        "DESCRIPTION" "inst"        "man"         "R"           "src"        
> document()
Updating BAMMtools documentation
Loading BAMMtools
Re-compiling BAMMtools
'/Library/Frameworks/R.framework/Resources/bin/R' --no-site-file --no-environ --no-save --no-restore  \
  --quiet CMD INSTALL '/Users/pascaltitle/bammtools/BAMMtools'  \
  --library='/var/folders/vv/by9k6s8n5g52vgc30x17cj1h0000gn/T//RtmpXq5uNC/devtools_install_6cad6b4e93bf'  \
  --no-R --no-data --no-help --no-demo --no-inst --no-docs --no-exec --no-multiarch --no-test-load 

* installing *source* package ‘BAMMtools’ ...
ERROR: a 'NAMESPACE' file is required
* removing ‘/private/var/folders/vv/by9k6s8n5g52vgc30x17cj1h0000gn/T/RtmpXq5uNC/devtools_install_6cad6b4e93bf/BAMMtools’
Error: Command failed (1)

Any idea why this isn't working?

josephwb commented 8 years ago

Are you on the master branch, or the devtools branch?

ptitle commented 8 years ago

I switched to the devtools branch.

Your branch is up-to-date with 'origin/devtools'.

josephwb commented 8 years ago

Hmm. Did you install devtools with all of its dependencies? This works fine for me. Maybe @jonchang can try?

josephwb commented 8 years ago

This is what my seesion looks like:

> require(devtools)
Loading required package: devtools
> sessionInfo()
R version 3.2.5 (2016-04-14)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Debian GNU/Linux 8 (jessie)

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] devtools_1.10.0

loaded via a namespace (and not attached):
[1] memoise_1.0.0 digest_0.6.9 
ptitle commented 8 years ago

This is what I get:

> sessionInfo()
R version 3.2.4 (2016-03-10)
Platform: x86_64-apple-darwin13.4.0 (64-bit)
Running under: OS X 10.11.4 (El Capitan)

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] devtools_1.11.1

loaded via a namespace (and not attached):
[1] magrittr_1.5   tools_3.2.4    withr_1.0.1    roxygen2_5.0.1 Rcpp_0.12.4    memoise_1.0.0 
[7] stringi_1.0-1  stringr_1.0.0  digest_0.6.9  
josephwb commented 8 years ago

Hmm. I upgraded everything, and it still works for me (both in rstudio gui and commandline).

ptitle commented 8 years ago

I'm getting the same behavior on by ubuntu machine:

In R:

> list.files()
[1] "data"        "DESCRIPTION" "inst"        "man"         "R"          
[6] "src"        
> document()
Updating BAMMtools documentation
Loading BAMMtools
Re-compiling BAMMtools
'/usr/lib/R/bin/R' --no-site-file --no-environ --no-save --no-restore --quiet  \
  CMD INSTALL '/home/pascaltitle/bammtools/BAMMtools'  \
  --library='/tmp/RtmpoSNOwv/devtools_install_65b7653fe138' --no-R --no-data  \
  --no-help --no-demo --no-inst --no-docs --no-exec --no-multiarch  \
  --no-test-load 

* installing *source* package ‘BAMMtools’ ...
ERROR: a 'NAMESPACE' file is required
* removing ‘/tmp/RtmpoSNOwv/devtools_install_65b7653fe138/BAMMtools’
Error: Command failed (1)
> sessionInfo()
R version 3.2.5 (2016-04-14)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 16.04 LTS

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] devtools_1.11.1 MASS_7.3-45    

loaded via a namespace (and not attached):
[1] magrittr_1.5   tools_3.2.5    withr_1.0.1    roxygen2_5.0.1 Rcpp_0.12.4   
[6] memoise_1.0.0  stringi_1.0-1  stringr_1.0.0  digest_0.6.9  
josephwb commented 8 years ago

Ok, I can recreate this now. It has to do with the deleted .o and .so files. Looking for a solution.

josephwb commented 8 years ago

Ok, so I guess you just need to make sure you have the .o and .so files before doing the silly experiment. Reset your branch:

git reset --hard

Go into R and do:

require(devtools);
document();

This will compile the .o and *.so files. With this present, you can do delete the NAMESPACE and man/ files, and recreate them with:

require(devtools);
document();
josephwb commented 8 years ago

Yeah,so I guess you can't be missing both NAMESPACE and compile files, but missing either is fine.

josephwb commented 8 years ago

I hope it is clear this is all a little moot: you would never go and delete everything: rather, document() will create/delete things as necessary.

ptitle commented 8 years ago

Thanks that fixed the problem.

ptitle commented 8 years ago

Thanks Joseph, this looks really useful, I'm going to merge it to master.