ropensci / neotoma

Programmatic R interface to the Neotoma Paleoecological Database.
https://docs.ropensci.org/neotoma
Other
30 stars 16 forks source link

Added more import statements, fixing keywords. #140

Closed SimonGoring closed 10 years ago

gavinsimpson commented 10 years ago

@SimonGoring the imports from the packages need to more more explicit that the entire package. Use @importFrom pkg fun1 fun2 not @import pkg. At the very least the imports you added in this PR will cause warnings under R CMD check as they will override pre-existing imports we made selectively in other functions.

SimonGoring commented 10 years ago

Yep, I realized that, I'm working on it! On Sep 17, 2014 5:27 PM, "Gavin Simpson" notifications@github.com wrote:

@SimonGoring https://github.com/SimonGoring the imports from the packages need to more more explicit that the entire package. Use @importFrom pkg fun1 fun2 not @import pkg. At the very least the imports you added in this PR will cause warnings under R CMD check as they will override pre-existing imports we made selectively in other functions.

— Reply to this email directly or view it on GitHub https://github.com/ropensci/neotoma/pull/140#issuecomment-55970325.

karthik commented 10 years ago

What packages are these? and how many functions are you importing?

SimonGoring commented 10 years ago

Only ever one or two, getform from RCurl, one fro. RJSONIO, and then one or two *ply commands at most from plyr. We use melt and dcast in one function as well.

Thoughts? On Sep 17, 2014 6:27 PM, "Karthik Ram" notifications@github.com wrote:

What packages are these? and how many functions are you importing?

— Reply to this email directly or view it on GitHub https://github.com/ropensci/neotoma/pull/140#issuecomment-55976197.

gavinsimpson commented 10 years ago

@karthik regardless of the number, good practice would be to selectively import only those functions used within a function.

@SimonGoring we should try to not use plyr, for efficiency reasons. The equivalent of llply() is lapply(). I got rid of all plyr functions at one point but some have crept back in since then.

karthik commented 10 years ago

The current recommendation is to even avoid what Gavin suggested, i.e.

@importFrom pkg fun1 fun2

Instead, if you just need a small number of functions from a package, just list the package in Imports and directly reference the functions using the :: operator. Hadley says:

Alternatively, though no longer recommended due to its poorer readability, use @importFrom, e.g., @importFrom pgk fun, and call the function(s) without ::.

So if you are using getURL from RCurl, in your code you would reference it as:

RCurl::getURL

That is the best suggested approach for now.

karthik commented 10 years ago

PS: I am not suggesting using

@import httr

for example.

gavinsimpson commented 10 years ago

@karthik It is important to remember that that is Hadley's opinion and he is nothing if not opinionated. As far anything is recommended when it comes to R, we must consult the Writing R Extensions manual, which states:

If a package only needs a few objects from another package it can use a fully qualified variable reference in the code instead of a formal import. A fully qualified reference to the function f in package foo is of the form foo::f. This is slightly less efficient than a formal import and also loses the advantage of recording all dependencies in the NAMESPACE file (but they still need to be recorded in the DESCRIPTION file).

(Emphasis mine)

I also don't agree with Hadley on the readability issue. Peppering one's code with pkg:: is hideous to my eyes. As we're using roxygen, I find a clear statement about what functions are imported by the current function is exceedingly clear, avoids polluting code with pkg::, and doesn't incur the minor efficiency penalty.

Regarding httr, I was going to enquire about whether this was something that people were using now in other rOpensci packages? If there were any advantages to it?