ropensci-review-tools / pkgstats

Historical statistics of every R package ever
https://docs.ropensci.org/pkgstats/
17 stars 1 forks source link

Demonstration isn't working because DESCRIPTION file entries are converted to factors #38

Closed GFabien closed 2 years ago

GFabien commented 2 years ago

Hi! I wanted to try the package, so I started by running the Demonstration code given in the README.

Unfortunately, this code returned an error related to strsplit being run on an object which is not a string. I tracked it down to the function desc_stats that read the DESCRIPTION file and extracted the information from the different fields of the file using the function read.dcf. It seems that this leads to creating a data frame where every column is a factor. Hence, applying strsplit on the data frame values raises an error (line 68 when parsing Imports).

Here is the code to reproduce the error:

library(pkgstats)
packageVersion("pkgstats")
#> [1] '0.0.3.88'
tarball <- system.file ("extdata", "pkgstats_9.9.tar.gz", package = "pkgstats")
system.time (
  p <- pkgstats (tarball)
)
#> Error in pkgstats(tarball): impossible de trouver la fonction "pkgstats"
#> Timing stopped at: 0 0 0

Thank you for your help! Fabien

mpadge commented 2 years ago

Thanks @GFabien, that's an important catch. See ?options for why that happens - prior to R4.1, R defaulted to stringsAsFactors = TRUE, and so by default converted all strings to factors. If you upgrade your version of R, that error should disappear. That said, we should catch and appropriately process that error here, for which there are two options:

  1. EnforceDepends: R (>=4.1.0) in the package DESCRIPTION file, so you'd get a message to upgrade R when you tried installing; or:
  2. Change the package internals here to avoid converting strings to factors for people using older versions of R.

@maelle Interested to hear your preference here, especially noting that this package is intended to be as current as possible, and is also likely to enforce 4.1 once 4.2 is released, to enable native pipes anyway. I'll likely implement option 2 anyway, just to cover all bases, but interested to hear your thoughts?

maelle commented 2 years ago

As this is a package aimed at package developers I think it's fine to depend on a recent R version.

GFabien commented 2 years ago

Thank you @mpadge! It works like a charm after updating my R version.

mpadge commented 2 years ago

Great to hear @GFabien, but i hard-coded stringsAreFactors = FALSE throughout anyway, so should now work even on R<=4.0. I'll close now, since depending on specific R version will be addressed later anyway. Thanks!