Add parameters in station_search() to search by start and end dates

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

Add parameters in station_search() to search by start and end dates #75

Closed nicholas512 closed 5 years ago

nicholas512 commented 5 years ago

Description

Add two new parameters to station_search() to restrict dates

Related Issue

issue #35.

Example

stations_search(name='ottawa', starts_before=1950, ends_after=2010)

steffilazerte commented 5 years ago

I'm away at a conference just now but will review when I get the chance, thanks!On Aug 27, 2019 12:42 PM, Nick notifications@github.com wrote:Description Add two new parameters to station_search() to restrict dates Related Issue issue #35. Example stations_search(name='ottawa', starts_before=1950, ends_after=2010)

You can view, comment on, or merge this pull request online at: https://github.com/ropensci/weathercan/pull/75

Commit Summary Add parameter in station_search() to search by start and end dates

File Changes

M
DESCRIPTION
(2)

M
NEWS.md
(4)

M
R/stations.R
(16)

Patch Links: https://github.com/ropensci/weathercan/pull/75.patchhttps://github.com/ropensci/weathercan/pull/75.diff

—You are receiving this because you are subscribed to this thread.Reply to this email directly, view it on GitHub, or mute the thread.

nicholas512 commented 5 years ago

Great! It's not a big change, but hopefully it is a step to the v1.0 release. Also, first time opening a pull request, so hopefully everything works smoothly. Awesome package by the way!

boshek commented 5 years ago

@nicholas512

Nice PR. Two things I would recommend to get this PR into a slightly more reviewable state.
1. Try running devtools::document() on the package so that the documentation in the roxygen code is actually converted into .Rd files.
2. Add a test to the test suite that tests the desired behaviour. Let us know if you want any assistance with that.
3. Before pushing run devtools::check() to make there aren't any issues.

nicholas512 commented 5 years ago

Thanks for the feedback @steffilazerte and @boshek. I'm just getting back from holidays and in the midst of starting a new job, but will aim to fix those issues as soon as I have a moment.

I went back and forth about whether to use inclusive or exclusive inequalities, but I agree that the points you've outlined show that >= is the better choice. As for a parameter name, I'm happy with starts_latest but will see if I can think up a less wordy alternative.

steffilazerte commented 5 years ago

By the way, I have a deadline of Sept 27th to push a new version of weathercan to CRAN (an update in one of our dependencies creates problems if a user has that version).

I'm hoping to get those fixes as well as the climate normals figured out by then. You can either try to get your edits in by then too, or wait until I've got the newest (unbroken) version submitted to CRAN and use that as your base to make edits to. That will mean we'll have to wait another couple of months before we can add your fix to the CRAN version, but better than doing a rush job when you're already busy!

nicholas512 commented 5 years ago

The 27th should be no problem. For the testthat tests, I had a look at the ones that are already there and I'm fairly sure I'll be able to figure out the syntax. Do I understand correctly that these just verify the expected output for a few typical and outlying test cases? For example, checking that a request for stations starting in 2000 and ending in 1990 would return an empty list?

steffilazerte commented 5 years ago

Exactly. Sometimes I'll use a manual filter on the stations data frame and compare it to the output of the stations_search function to make sure they line up, but as long as the expected output is tested, that's fine!

This is especially useful for when we make changes to the functions in the future, as these tests will let us know if we've unwittingly broken something.

boshek commented 5 years ago

Just to add about tests, if you can add even the tiniest edge case the test is worth it. On the scale of tests for weathercan, I don't think you can really test too much.

nicholas512 commented 5 years ago

I'm not sure what caused those checks to error - I was getting a note during devtools::check() no visible binding for global variable 'end'. I didn't think it would be an issue, but perhaps that was the problem. I've just pushed a quick patch based on advice here that gives me 0 warnings/errors/notes on check() so if that doesn't do it I'm not too sure what the problem is. Thanks for your help and patience!

steffilazerte commented 5 years ago

Hi @nicholas512 thanks for making all the changes!

It looks good, I made just a couple of tweaks:

use as.character() inside the as.numeric() as a bit of a fail safe in case the users passes a factor instead of a number of character)
If we use the try() function I generally check for a try-error class (I'm pretty sure there are more elegant ways of checking for errors, but this is the method I've landed on).
I added one more test which checks for the use of both start and end dates at the same time.

I wouldn't worry about the note, I generally include the variables which trigger that note in the .onLoad() function (in weathercan-pkg.R if you're curious). But I've since started to think that the better way is to use .data in tidyverse functions, eg:

dplyr::filter(weather, .data$start <= start)

But I'll have to switch the whole package over, so I'll deal with this later.

Thanks again!