Integrate updated version of the package.

[npnlee85]

This package was originally initiated by Scott Chamberlain in 2013. Not much work was done on it until a few years ago. Since then, the NPN has worked on a separate repo/fork improving this package and bringing it up-to-date with changes and improvements to our data services. As such, this pull request captures years of changes and is a complete overhaul of the package's functionality and documentation. The intention is that going forward changes can be pulled up to the ropensci repo more frequently and more discretely.

[sckott]

thanks, having a look

[npnlee85]

Still working through this but a few questions/comments: 1) Re the docs folder: I'd like to keep that if it's not an issue, because we're using it for the Github pages associated with the forked repo: https://usa-npn.github.io/rnpn/ Maybe that's a little redundant? 2) Re the tests and CRAN: how quick is CRAN to pull a package? In many cases, it doesn't really make sense to cache responses from the server since in many ways, what's being tested is the response. On the other hand, I don't want the tests always taking forever, esp. on CRAN, invalidating test results, or throwing false negatives if a CRAN test is running when we coincidentally are having an outage. Thoughts on this?

[sckott]

sure, can keep docs folder, the main difference would be different urls of course, but styling will differ

you really don't want any failures in tests/examples on cran as they can quickly email after a failure and ask that it be fixed or the package be removed from cran within 2 weeks. id strongly suggest not running examples OR tests on cran that make real HTTP requests, b/c 1) their server may have a problem, 2) their internet may have a problem, 3) npn api/server may have a problem, etc.

what's being tested is the response.

Yes, but you're also testing the code that checks the response, munges the data into a data.frame, etc. What vcr does is remove the part where you're currently testing the http request/response behavior.

I'd suggest not allowing real http requests on CRAN BUT definitely do real http requests on your CI system of choice and locally of course

Ultimately its up to you - just communicating my experience with cran

[npnlee85]

Currently attempting to incorporate vcr into the unit tests as discussed. However, I'm running into a problem wherein the particularly heavy requests aren't being cached (vcr generates an empty file). Per the documentation, vcr only works with httr and crul libraries but this part of the package is using curl instead; see this line - https://github.com/ropensci/rnpn/blob/2795dc7b81bdcc525ec7ea784b666b339348bb1e/R/npn_data_download.R#L671

I'm not sure if the problem is that I'm using curl, or because I'm streaming the response, OR if it's something else all together and wondering if you have any ideas @sckott . I could see changing this code to use crul or httr or I could hand mock vcr yml files but that sounds tedious.

[sckott]

ah sorry, vcr does not work with curl, sorry I should have seen that you were using curl and not recommended it. Yeah, if you switch to crul or httr you could use vcr.

[npnlee85]

Resolved most of the rest of the comments except for the bit about the vignettes. Honestly, not sure what to do with it, might need some guidance. Rest of the comments should be sorted, submitted request for re-review.

[npnlee85]

Putting in a new review request. A few comments to accompany that: 1) I believe this has been transitioned over correctly to using Github Actions. I've removed any references to travis, and I also removed appveyor simply because I didn't put it there, I don't use it. If there's value in having a secondary CI service or if it's standard for ropensci, I don't mind putting it back. 2) Re your comment about the text fixtures: I believe that's already in place. Each of the unit tests is prefaced with a call to npn_set_env to standardize the unit tests to use the dev environment which should match the test fixtures.

[sckott]

Thanks, I'll have a look

[sckott]

LGTM