search

quandl / quandl-r

This is Quandl's R Package
https://cran.r-project.org/web/packages/Quandl/
Other
139 stars 55 forks source link

Ap 330/feature v3 #38

Closed ccleung closed 9 years ago

ccleung commented 9 years ago

WIP

RaymondMcT commented 9 years ago

xts needs to stay in depends as the package will break if it is moved into imports. If I move timeSeries into imports it will be automatically installed with Quandl. It is the intention to expand the return formats eventually to some of the others in R (its, irts, etc.) and I do not want them to be required installations with the package.

As well adding the lines

#' @import httr
#' @import jsonlite

Will load the entirety of httr and jsonlite into the global namespace, which I don't think is necessary.

dmpe commented 9 years ago

In regard to the second one: you can also use importFrom pck fnct.

-> timeseries: then it should be documented (+at least added to the suggested ones; saw later as it is already done; sorry)

ccleung commented 9 years ago

@dmpe Is there a reason why you want to import the namespace directives? we are hoping to following best practices seen in other popular R packages, e.g., devtools does not import package namespaces: https://github.com/hadley/devtools/blob/master/R/github.R#L10 and we prefer not to do so as well

dmpe commented 9 years ago

@ccleung

1. Yes. https://github.com/ropensci/onboarding/issues/17#issuecomment-116782430 (section Imports)

You have httr and jsonlite (and many others) as Imports in DESCRIPTION file, but they aren't listed in any @ import. Was there a reason behind this? Even though httr and jsonlite are namespaced, those will still fail for users that don't have those pkgs installed.

2. best practices seen in other popular R packages: Well, dplyr is very popular.

https://github.com/hadley/dplyr/search?utf8=%E2%9C%93&q=importFrom&type=Code https://github.com/ropensci/geojsonio/search?utf8=%E2%9C%93&q=import&type=Code https://github.com/ropensci/fulltext/search?utf8=%E2%9C%93&q=import