search

rOpenGov / fmi

Finnish Meteorological Institute open data API R client
Other
10 stars 7 forks source link

Make R CMD check pass #12

Closed jlehtoma closed 7 years ago

jlehtoma commented 7 years ago

Re-opening this issue because of NOTEs in check. As stated elsewhere:

NOTEs: Mild problems. If you are submitting to CRAN, you should strive to eliminate all NOTEs, even if they are false positives. If you have no NOTEs, human intervention is not required, and the package submission process will be easier. If it’s not possible to eliminate a NOTE, you’ll need describe why it’s OK in your submission comments, as described in release notes. If you’re not submitting to CRAN, carefully read each NOTE, but don’t go out of your way to fix things that you don’t think are problems.

The NOTEs we're getting are caused by the non-standard evaluation syntax used by most notably in dplyr. Reading from the interwebs, there are workarounds to get rid of the NOTEs (i.e. using the underscore versions of dplyr functions), but especially mutate() / mutate_() takes a little more advanced alchemy. Two questions for @rOpenGov/fmi or anyone else who might have experience on the matter:

  1. Is it really necessary to get rid of NOTEs (because of CRAN)?
  2. If "yes" to question 1, then any tips on what's the easiest way of dealing with the issue?
jlehtoma commented 7 years ago

Related to #5

ilarischeinin commented 7 years ago

I agree with what is said there by Hadley Wickham in R packages. Even though it's a bit hacky, I think it makes sense to get rid of them. On that same page:

codetools::checkUsagePackage() is called to check that your functions don’t use variables that don’t exist. This sometimes raises false positives with functions that use non-standard evaluation (NSE), like subset() or with(). Generally, I think you should avoid NSE in package functions, and hence avoid this NOTE, but if you can not, see ?globalVariables for how to suppress this NOTE.

I know I used dplyr's mutate() in the fmi_stations() I just wrote, and I'm happy to rewrite that bit to avoid NSE if that's preferred. Or if sticking with that syntax, add a globalVariables() call. It maybe makes sense to place those calls as close as possible to wherever NSE is used in the code, rather than collecting everything in one call for the whole package?

(As an aside, it seems that Hadley used to think otherwise (see the top-voted comment) about globalVariables().)

jlehtoma commented 7 years ago

Funny fellow that Hadley... Intuitively, I would've called globalVariables() a hideous hack too, but it does seem like the simplest thing to do. I too use dplyr and insist of doing so, so I think there are few places in the packages this affects. Looking at ?globalVariables():

Repeated calls in the same package accumulate the names of the global variables.

so it can be called several times. I agree, placing them close to where they're called makes sense.

jlehtoma commented 7 years ago

I think you can't place globalVariables() within the function definition (e.g. fmi_stations()) where NSE takes place. Thus, the closest point might be just before the function definition (and the Roxygen2 docmentation).

jlehtoma commented 7 years ago

I went ahead and declared the globalVariables in two places. Version 0.1.15 now passes check without NOTEs, closing this issue.