ropensci / weathercan

R package for downloading weather data from Environment and Climate Change Canada
https://docs.ropensci.org/weathercan
GNU General Public License v3.0
102 stars 29 forks source link

Major update #42

Closed steffilazerte closed 6 years ago

steffilazerte commented 6 years ago

Fixes some issues introduced by the update to lubridate v.0.7.0 (now v.0.7.1). Adds some functionality and tweaks code for rOpenSci application. Also added codecoverage and allowed the R Devel tests to fail without reporting a total failure on Travis CI and AppVeyor.

There's also a draft of the paper for JOSS (too long?)

Would almost suggest going straight to master with this, so we can get on the rOpenSci application soon? I probably should have pushed some of these fixes to the devel branch long ago!

In Detail:

Changes

Bug fixes

codecov-io commented 6 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (devel@17f47f3). Click here to learn what that means. The diff coverage is 91.58%.

Impacted file tree graph

@@           Coverage Diff            @@
##             devel      #42   +/-   ##
========================================
  Coverage         ?   93.88%           
========================================
  Files            ?        5           
  Lines            ?      507           
  Branches         ?        0           
========================================
  Hits             ?      476           
  Misses           ?       31           
  Partials         ?        0
Impacted Files Coverage Δ
R/weathercan-pkg.R 0% <0%> (ø)
R/weather.R 94.92% <89.74%> (ø)
R/utils.R 94.11% <91.66%> (ø)
R/interpolate.R 94.94% <94.73%> (ø)
R/stations.R 99.07% <97.05%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 17f47f3...f4f084f. Read the comment docs.

steffilazerte commented 6 years ago

Re Sam's changes

Re Sam's comments/questions

stations_all(url = "ftp://client_climate@ftp.tor.ec.gc.ca/Pub/Get_More_Data_Plus_de_donnees/Station%20Inventory%20EN.csv")

Perhaps it was the shortened link? I didn't really like using a shortened link to begin with but I was following Hadley's suggestion for fixing extra long lines in the documentation (http://r-pkgs.had.co.nz/check.html). So I got rid of the long link in the documentation but the script now uses it directly ( 063aaf8)

Conclusions

boshek commented 6 years ago

And I don't think the README needs to be installed

What do you mean by that? CRAN builds README files locally so they have to runnable. Therefore this applies:

"Subdirectory tools is the preferred place for auxiliary files needed during configuration, and also for sources need to re-create scripts (e.g. M4 files for autoconf). "

I interpret that because README has to be built with pandoc on CRAN they want any images in the tools folder. FWIW, this is a new policy. Here is the message from CRAN last March (though I can't find the original):

It was recently pointed out to us that some README.html files (generated from the corresponding README.md ones) on the CRAN package web pages are incomplete, missing 'local' images not available from the web page and in most cases actually not even shipped with the package. This clearly should be changed, so we will move to using '--self-contained' for the pandoc conversion to ensure that the README.html files are "complete".

Of course, this implies that all 'local' images used in README.md are needed in the package sources.

If the images are also used for vignettes or Rd files, you can put them in the 'vignettes' or 'man/figures' directories. Otherwise, please put them in the top-level 'tools' directory, or a subdirectory of it.

The CRAN incoming checks in r-devel were changed to perform the pandoc conversion checks with '--self-contained', and hence will warn about missing images.

Pls ensure completeness in the next regular update of your package.

I just removed the date from DESCRIPTION because nothing has been release yet. I typically do that as a last step. So no problem with the date itself.

Everything else looks great.

steffilazerte commented 6 years ago

Okay that makes sense, thanks for clarifying. I think I've always thought of the README as something for github, but it is relevant to the package too.