Add interval check to weather_interp and stations_search

[farhanreynaldo]

Description

For weather_interp and stations_search function, check the interval parameter to ensure that the value is either hour, day, or month.

Related Issue

Fix #166

Example

Best Practices

The following have been updated or added as needed:
[ ] Documentation
[ ] Examples in documentation
[ ] Vignettes
[x] testthat Tests

[steffilazerte]

HI @farhanreynaldo,

Thank you for your contributions!

I have made a couple of suggestions which you can just commit directly (there should be a button that allows you to just add it to the PR). This adds the call. = FALSE argument which makes the error message to the user a bit 'cleaner' and avoids including information that isn't generally useful.

I appreciate that I haven't been very consistent about this, however, so you'll see instances in other places where I haven't done this either. A clear case of "do what I say and not what I do" 😆

As for tests:
First, note that I have been updating the testing in main, so you'll want to update your branch with main before proceeding (let me know if you need any help doing this).

Second, I recommend putting a test for the interpolation argument in tests/testthat/test_01_interpolate.R, you can add a test block right at the bottom, call it something like test_that("weather_interp checks arguments", {...

For the stations, put it in tests/testthat/test_03_station_dl.R, again at the bottom with a similar label.

If you haven't already, you can see the "in depth" section of the Contributing guide, as well as the Testing basics chapter of the R Packages book, and of course, ask me any questions.

Does that help you get started?

[farhanreynaldo]

Thank you, @steffilazerte, for the suggestions. I have a few follow-up questions:

• For the weather_interp function, is it correct that users can only select either hour or day intervals—but not month?
• For the stations_search function, is it correct that users can select multiple intervals, so we only need to check whether the interval values are valid (i.e., any of hour, day, or month)?
• I noticed that the stations_search function already includes an interval check via the check_int function. Should we remove this check in favor of the current changes?

[steffilazerte]

Hi @farhanreynaldo,

For the weather_interp function, is it correct that users can only select either hour or day intervals—but not month?

Yes, only hour or day, not month. To interpolate over month users would have to decide how to average the data first, and at that point they should probably think a bit harder about what they want and do it themselves.

For the stations_search function, is it correct that users can select multiple intervals, so we only need to check whether the interval values are valid (i.e., any of hour, day, or month)?

That is correct, by default the stations_search() function returns all intervals

I noticed that the stations_search function already includes an interval check via the check_int function. Should we remove this check in favor of the current changes?

Ah, whoops, you can see the results of non-systematic updates to an old package (started in 2017!). In this case, I would keep check_int() and just add the single hour/day interval checks to weather_interp().

Thank you!

[farhanreynaldo]

Noted, I already added the tests and used the existing interval check instead for stations_search.