gesistsa
Potential future candidates for refactoring arguments
Please specify whether your issue is about:
- [x] a possible bug
- [ ] a question about package functionality
- [ ] a suggested code or documentation change, improvement to the code, or feature request
If you are reporting (1) a bug or (2) a question about code, please supply:
- a fully reproducible example using a publicly available dataset (or provide your data)
- if an error is occurring, include the output of
traceback()run immediately after the error occurs- the output of
sessionInfo()
These are not bugs I encountered, but in reviewing import_methods.R it's clear that sooner or later somebody who uses certain formats will.
Rather than put in repetitive example code, I will just list below the reader functions that rio calls with an ... argument but which do not have a ... in their calling signatures. To replicate the bug, run import using any of these and include some argument that they do not accept (e.g. skip = 42).
.import.rio_* will allow you to use such an argument (as it should) and it will pass that argument to the target function via ... but the target function will error instead of ignoring it because it does not have an ... argument.
base::dget
foreign::read.epiinfo
foreign::read.systat
foreign::read.mtp
fst::read.fst
haven::read_sas
haven::read_xpt
hexView::readEViews
pzfx::read_pzfx
readODS::read_ods
utils::read.DIF
## session info for your system
Not applicable
This can be fixed either by removing ... from the function call or by wrapping the above functions in arg_reconcile(). I will be happy to submit a PR for this if there is interest.
@bokov Thank you so much for your work on this package.
I would argue that this is a feature according to the documentation, not a bug.
https://github.com/chainsawriot/rio/blob/8fd8ba734ac58286faca1c3672123bdb3a0d88eb/R/import.R#L8
Additional arguments passed to the underlying import functions. For example, this can control column classes for delimited file types, or control the use of haven for Stata and SPSS or readxl for Excel (.xlsx) format. See details below.
https://github.com/chainsawriot/rio/blob/8fd8ba734ac58286faca1c3672123bdb3a0d88eb/R/import.R#L10
This function imports a data frame or matrix from a data file with the file format based on the file extension (or the manually specified format, if \code{format} is specified).
We also listed out all the underlying functions. Therefore, when a user did this
tempfile <- tempfile(fileext = ".yml")
export(mtcars, tempfile)
import(tempfile, hello = 42)
This user should expect the "additional argument" hello will pass to the underlying function yaml::read_yaml; and like many wrappers, it should be an error. (similar to what Jenny Bryan said in this issue) Giving a warning, like the current arg_reconcile does, probably is not what one should expected.
Instead, the remapping behavior (in this case header, an "additional argument" to readODS::read_ods got mapped to col_names) is never documented. In this sense, it should be a "bug". At least a wat.
tempfile <- tempfile(fileext = ".ods")
export(head(mtcars), tempfile)
import(tempfile, header = TRUE)
But not this:
tempfile <- tempfile(fileext = ".xlsx")
export(head(mtcars), tempfile)
import(tempfile, header = TRUE)
The correct course of action is either abandon all remapping behaviours (which is not a bad idea actually, especially for the API-breaking v1.0.0); or to document all remapped parameters, not just which. And let ... be ....
