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

remove stations_search() ability to search by coordinate. Removes sp … #46

Closed boshek closed 6 years ago

boshek commented 6 years ago

…dependency and would close #45

Merging to steffi_exp as I think this is where you are doing much of the review work.

This is an aggressive fix to the problem of importing the sp package. And it is a real shame because it is quite nice code. I would argue though that this reaches beyond the scope of the weathercan package and searching for stations by coordinates can be done with the sp (or preferably the sf) package as users zero in on exactly what stations they want. As I mentioned in the issue, I think importing sp is such a high cost for only this functionality.

codecov-io commented 6 years ago

Codecov Report

Merging #46 into steffi_exp will decrease coverage by 0.66%. The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           steffi_exp      #46      +/-   ##
==============================================
- Coverage       93.86%   93.19%   -0.67%     
==============================================
  Files               5        5              
  Lines             505      485      -20     
==============================================
- Hits              474      452      -22     
- Misses             31       33       +2
Impacted Files Coverage Δ
R/stations.R 96.55% <100%> (-2.52%) :arrow_down:

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 ccbabcc...e2cbded. Read the comment docs.

steffilazerte commented 6 years ago

I don't think I'm happy with simply removing that option. I'd much rather make the sp package a suggests, then check for the package before using coordinates. Then if the user doesn't have it, they'll get a message saying to install sp. I'd also still like to look into a way of including this functionality, perhaps with another package. But haven't had the chance to look into it yet.

nicholasjhorton commented 6 years ago

I would also +1 the suggests proposal to simplify the install but keep this feature.