Get neotoma up onto cran

SimonGoring commented 10 years ago

Not sure what needs to be done for unit testing. but:

[x] Unit tests need to be completed
[x] Bug fixes need to be completed
[x] Other (non-essential?) cleanup needs to be undertaken
[x] Documentation needs to be proofread

sckott commented 10 years ago

hey simon. we do write tests, but usually don't have them run on CRAN, that is, we put them in inst/tests/, and use them for local testing, but when you submit to CRAN, don't allow tests to run, at least those that make API calls to the web.

karthik commented 10 years ago

Thanks for posting this,Simon. Will follow up shortly.

@Schamberlain this follows from a phone call with simon a little while ago.

On Fri, Nov 1, 2013 at 12:53 PM, Scott Chamberlain notifications@github.com wrote:

hey simon. we do write tests, but usually don't have them run on CRAN, that is, we put them in inst/tests/, and use them for local testing, but when you submit to CRAN, don't allow tests to run, at least those that make API calls to the web.

Reply to this email directly or view it on GitHub: https://github.com/ropensci/neotoma/issues/60#issuecomment-27595960

gavinsimpson commented 10 years ago

Scott is right of course, but I would argue that we should also have tests which fake the calls and use stored output as if the API calls were made and then run the functions that process those results. That might need a bit of rearrangement in the package code. For example, you would need to separate the code that does the API call from code that processes the results. Hence you might have

foo <- function(x, ...) {
    ## do API call
    res <- call_foo_api(x, ...)

    ## process JSON
    out <- proc_foo_api_call(res)
    out
}

Then the tests could run proc_foo_api_call() by supplying it with the JSON object saved out to RData that would have arisen from the API call had it been run.

That's a lot more effort of course, but ultimately changes to the package which break things will get spotted more quickly.

(Of course, all the above would be done with far fewer underscores ;-)

sckott commented 10 years ago

ah, okay, wasn't aware of earlier convos.

@gavinsimpson Good idea, we should think about doing that for sure. Though the underscores above should be longer, like call______foo______api instead of call_foo_api - I kid :)

gavinsimpson commented 10 years ago

Adding this here: as of today there are the following issues which also need addressing before the package can go to CRAN:

[x] checking S3 generic/method consistency ... WARNING This is argument difference between generics and methods and is being tracked in Issue #117
[x] * checking R code for possible problems ... NOTE
```
compile_list: no visible binding for global variable ‘pollen.equiv’
get_dataset.site : pull_site: no visible binding for global variable
  ‘neotoma.form’
get_datasets: no visible binding for global variable ‘gp.table’
get_sites: no visible binding for global variable ‘gp.table’
param_check: no visible binding for global variable ‘gp.table’
```
When I checked one or two of these, this was just an issue in that codetools doesn't know that there are side effects of loading objects via say data(). So I think these are false positives but good to check each on. Note that there is a convention to record these globals but I forget what it is now...?

One way of dealing with this is globalVariables(), though see recent thread on R-Devel on this issue.
[x] checking for code/documentation mismatches ... WARNING Need documentation for check.tables()
[x] checking Rd \usage sections ... WARNING Lots of documentation needed, especially for the new methods.
[x] checking data for non-ASCII characters ... WARNING
```
Note: found 814 marked UTF-8 strings
Warning: found non-ASCII strings....
```
Need to work out what the correct way of dealing with this is; something will be in Writing R Extensions on this... FIXED at least partially. Need to check with CRAN that a NOTE about UTF-8 is OK. The WARNING is now gone though, so yay.
[x] checking for unstated dependencies in tests ... WARNING
```
'library' or 'require' call not declared from: ‘testthat’
```
@sckott's Issue addresses #115 this. FIXED
[x] checking PDF version of manual ... WARNING
```
LaTeX errors when creating PDF version.
This typically indicates Rd problems.
LaTeX errors found: ...
```
This is probably just some bad markup in the roxygen tags. Best to go through this once all the documentation has been written so fixes are done on the file versions rather than risk reintroducing errors as the documentation is edited. FIXED

SimonGoring commented 9 years ago

I think this is all cleared up, except for the issues between Travis and Appveyor as indicated in issue #160.

gavinsimpson commented 9 years ago

We've still got issues.

[x] need to make gp.table be a sysdata object and not use data(gp.table() in the code/function,

[x] need to wrap the text/code in the example for get_download() in particular this line which R CMD check is complaining about

Rd file 'get_download.Rd':
\examples lines wider than 100 characters:
t8kyr.datasets <- get_dataset(taxonname='*Pseudotsuga*', loc=c(-150, 40, -120, 60), ageold=20000, ageyoung=10000)

[x] still a note about non-ASCII characters in data (presume it means gp.table?)

SimonGoring commented 9 years ago

I'll go through these. It's dinner & family time now.

On Fri, Feb 20, 2015 at 4:53 PM, Gavin Simpson notifications@github.com wrote:

We've still got issues.

need to make gp.table be a sysdata object and not use

data(gp.table() in the code/function,

need to wrap the text/code in the example for get_download() in particular this line which R CMD check is complaining about

Rd file 'get_download.Rd': \examples lines wider than 100 characters: t8kyr.datasets <- get_dataset(taxonname='Pseudotsuga', loc=c(-150, 40, -120, 60), ageold=20000, ageyoung=10000)

[] still a note about non-ASCII characters in data (presume it means gp.table?)

— Reply to this email directly or view it on GitHub https://github.com/ropensci/neotoma/issues/60#issuecomment-75334549.

gavinsimpson commented 9 years ago

@SimonGoring you can delegate remember! Add an issue for each outstanding problem and allocate someone to fix it. Worst that can happen is that said person declines. I've held off doing anything of late as you been touching the code base all over and I wasn't sure what you were and weren't working on.

SimonGoring commented 9 years ago

I'm the king of this castle and I'll do what I want! :)

Just pinged you on the extractor stuff. I don't think it's the gp.table that's the problem, I think it's things like isoetes that get pinged, but I'll try to track it down. The gp.table is a list of country names.

gavinsimpson commented 9 years ago

gp.table is a separate issue from the non-ASCII strings; you shouldn't be doing data(foo) in R function code in packages now. You need sysdata for that.

SimonGoring commented 9 years ago

Okay, I'll look into sysdata.

SimonGoring commented 9 years ago

Okay, I've cleared up the sysdata/data issue. Finished with commit https://github.com/ropensci/neotoma/commit/87ad4ad5bea9b0aafafec0d3102c412738ae3646. Just the ASCII issues left now.

SimonGoring commented 9 years ago

It looks like the non-ASCII characters error is resolved. Not sure where that happened, I'm trolling through the logs but haven't found it yet.

SimonGoring commented 9 years ago

Alright. Right now it looks like everything is cleared up. I've updated my local R version and am running the checks right now, but things all seem to be passing. The last outstanding issue is the Appveyor and Travis problem (Issue #160) but these problems seem to be tied to the tests.

Given that all tests pass locally and we're not getting flagged for any Errors, warnings or notes, I'm going to try to release the package using devtools::release(). I'm just doing some finishing touches.

SimonGoring commented 9 years ago

So, things are running on Linux according to Travis, I don't know if @gavinsimpson needs to compile, but is certainly welcome to. There are two notes that seem to remain in the Travis build, and I'm not sure how to resolve them. Are these critical issues @karthik or @gavinsimpson? or will CRAN accept the build as is?

Sorry for all the pushes lately, I can't compile on Linux here, and the Travis builds seem to be giving me more extensive notes then the devtools::check() results I'm getting on Windows.

gavinsimpson commented 9 years ago

@SimonGoring Those two NOTEs are fine. The first you'll always get with --as-cran as it just flags up the maintainer and the licence of the package as a quick check for the CRAN bods. The second is just a Travis config issue - the system doesn't seem configured with a library location or repository so the package dependency checks haven't been completed there fully. This will go away on CRAN when they test.

I've compiled and checked under R 3-2-patched and R-Devel and I am seeing no issues either. If you aren't seeing anything at your end, Win Builder isn't seeing anything, then you are ready to brave the CRAN folk with neotoma.

SimonGoring commented 9 years ago

We're up on CRAN so I'm going to close this. Thanks everyone!

sckott commented 9 years ago

:rocket:

ropensci / neotoma

Get neotoma up onto cran #60

hey simon. we do write tests, but usually don't have them run on CRAN, that is, we put them in inst/tests/, and use them for local testing, but when you submit to CRAN, don't allow tests to run, at least those that make API calls to the web.

data(gp.table() in the code/function,