Closed Hugovdberg closed 6 years ago
@Hugovdberg Is this ready for review or still working on it?
I think it is ready for review, I think further optimization and cleaning is possible, but I prefer to do it after you reviewed it. Keep in mind I propose to only introduce this in 1.0.
Also, the dependency on tibble
and R6
should be too much of a problem because they are both dependencies of dplyr
, which will be introduced with the tidyverse
anyway. R6
classes have a nice lightweight profile, and act more like classical OOP where data and methods are kept together. I first tried to implement it as an S3
class but ended up needlessly cluttering the interface with a lot of auxiliary methods that R6
just doesn't need.
Major change of the interface, this is a pull request against the
release-1.0.0
branch so we can clean up the current 0.8.2 release and finalise it into the 0.8.3 release. This feature needs some thorough testing and should not be released in 0.8.3 in my opinion.Types of change
Pull request checklist
man
Proposed changes
.add.extension
that stores the passed in reader with the matching extension. Unlike previous versions an actual reference to the function is stored instead of a character string that is evaluated when loading the data. To be able to store and display the reader nicely in a data.frame I created a newR6
class that stores the function and the name. I then changedlist.data
to actually load it into a tibble because tibbles handle non standard columns better. Alsoload.project
actually calls thereader
function inside theR6
object instead of looking up the function by name..add.extension
. This way the reader and the code adding it to the list of available readers is more or less self contained. Thepreinstalled.readers
list is now a function that can actually be used to inspect which readers are available. Perhaps this function should be renamedavailable.readers
.R6
andtibble
are now actual dependencies of the package I added them to theImports
section inDESCRIPTION
xls.reader
andxlsx.reader
are actually the same reader since we migrated toreadxl
I removed thexlsx.reader
and added both extensions to thexls.reader
.