search

jc-castillo / vaccineEarlyInvest

2 stars 1 forks source link

Use data/, not system.file() #16

Open wwiecek opened 4 years ago

wwiecek commented 4 years ago

Just flagging the alternative to using system.file() for built-in datasets would be to simply use data/ folder, it's more intuitive and user-friendly way to include countryData and vaccineSummary tables. I'm sure Junyi could do it quick.

THis is not critical but flagging this as I saw some problems with system.file() usage (e.g. in examples), which may make a CRAN submission more difficult.

wwiecek commented 4 years ago

Comments from @Junyi-Que :

First of all, given that we're uisng raw data, it's supposed to be put in inst/extdata instead of data and it's conventional to use system.file() to use such data. I googled but there is no clear answer to why system.file() gives some problem on your end, which didn't happen on my end. The path of examples when running check is somewhat different from the path when actually using the library, as the check procedure creates a temporary check file in which files are arranged differently, e.g., "vaccineEarlyInvest/extdata/vaccinesSummaryAug20.csv". I think it's better to keep the examples with system.file() or inst/extdata but also with dontrun. You might wonder why (so do I) inst/extdata instead of extdata, given that files in inst/ will be moved one level up once loading the package. I tried both after installing and loading the package and it turned out the former worked.

jc-castillo commented 4 years ago

I don't mind where the data is. So @Junyi-Que, feel free to do what googling tells you is the right thing to do. Just make sure to flag it the moment you make changes.

wwiecek commented 4 years ago

@Junyi-Que I'd suggest we wait until we try to submit the package to CRAN. If there are no problems on the first submission then no need to do this extra work, but I was flagging this up for the future because I thought it may generate some problems.