fs
fs copied to clipboard
r-lib
Throw error on base R functions
base_error <- function(env = parent.frame()) {
err <- function(...) stop("Use fs instead!", call. = FALSE)
env$file.rename <- err
env$file.create <- err
env$file.exists <- err
...
}
If you want to make sure that you've eliminated all base R file system operations.
An alternative would be a scanner tool that is not active while your package is running in production. That would allow false positives, and the odd cases when you actually want some base calls. People could run it fro their package tests.
Yeah, we could make a linter for it pretty easily at least for the simple cases; e.g. regular calls and possibly special case do.call() as well?
I guess we already have this right? https://github.com/MangoTheCat/goodpractice/blob/master/R/my_linters.R#L114
e.g. when run from the devtools project directory.
lintr::lint_package(
linters = list(base_fs_linter = function(source_file) {
goodpractice:::dangerous_functions_linter(source_file,
funcs = c("file.rename", "file.create", "file.exists"),
type = "warning",
msg = "Avoid base filesystem functions",
linter = "base_fs_linter")
}))
R/check-devtools.r:86:8: warning: Avoid base filesystem functions
if (!file.exists(news_path))
^~~~~~~~~~~
R/check-devtools.r:90:8: warning: Avoid base filesystem functions
if (!file.exists(ignore_path)) {
^~~~~~~~~~~
R/check-devtools.r:106:6: warning: Avoid base filesystem functions
!file.exists(news_rd_path),
^~~~~~~~~~~
R/create.r:10:8: warning: Avoid base filesystem functions
if (!file.exists(parent_dir)) {
^~~~~~~~~~~
R/create.r:16:8: warning: Avoid base filesystem functions
if (!file.exists(path)) {
^~~~~~~~~~~
R/create.r:69:7: warning: Avoid base filesystem functions
if (file.exists(desc_path)) return(FALSE)
^~~~~~~~~~~
...
We could probably define this linter in fs, to make it easy for people to use.
Yeah, I think the generic "dangerous function linter" could be in lintr, probably. It does not find do.call(), etc. currently, of course.
On a second thought, I like the original base_error() idea as well, but maybe I would only turn these errors on for the test cases?
Btw. this would be also great for rlang, and probably other packages as well.
@jimhester We're seriously considering doing this in usethis.
I remember you helping me lint some package for base R file handling functions. Probably usethis?
But I can't turn up a branch for that. Do you remember / have a record of it? Because it contained the necessary "dataset" of functions.
The list of base functions linked above is missing basename() and dirname().
This served me pretty well today:
base_functions <- c(
"Sys.chmod",
"Sys.readlink",
"Sys.setFileTime",
"Sys.umask",
"dir",
"dir.create",
"dir.exists",
"file.access",
"file.append",
"file.copy",
"file.create",
"file.exists",
"file.info",
"file.link",
"file.mode",
"file.mtime",
"file.path",
"file.remove",
"file.rename",
"file.size",
"file.symlink",
"list.files",
"list.dirs",
"normalizePath",
"path.expand",
"tempdir",
"tempfile",
"unlink",
"basename",
"dirname")
function_linter <- function(source_file, funcs, type,
msg, linter) {
bad <- which(
source_file$parsed_content$token == "SYMBOL_FUNCTION_CALL" &
source_file$parsed_content$text %in% funcs
)
lapply(
bad,
function(line) {
parsed <- source_file$parsed_content[line, ]
msg <- gsub("%s", source_file$parsed_content$text[line], msg)
lintr::Lint(
filename = source_file$filename,
line_number = parsed$line1,
column_number = parsed$col1,
type = type,
message = msg,
line = source_file$lines[as.character(parsed$line1)],
ranges = list(c(parsed$col1, parsed$col2)),
linter = linter
)
}
)
}
base_fs_funs <- function(path = ".") {
lintr::lint_package(path = path,
linters = list(base_fs_linter = function(source_file) {
function_linter(source_file,
funcs = base_functions,
type = "warning",
msg = "Avoid base filesystem functions (%s)",
linter = "base_fs_linter")
}))
}
I don't think a linter is quite strong enough; for packages I'd prefer to get an error message rather than having to remember to run a linter. (i.e. we should do both)
Given that we now have github actions, I'm now in favour of the lintr approach.
usethis is now using the lintr approach with GHA:
https://github.com/r-lib/usethis/blob/master/.github/workflows/lint-undesirable-functions.yaml
~Currently using a branch in my fork of lintr, but presumably I will eventually be able to use a (dev) version of lintr itself.~
We seem to have agreed that linting is the way to go and usethis provides a good list of which base R functions to look for. However, the way @Hadley wired linting into devtools is better than what I've currently got in usethis. The devtools linting is about using cli, not fs, but the approach transfers regardless. So I just dropped by here to note that the current best solution to this issue would be to plug the undesirable function info from usethis into the linting approach from devtools.
Maybe the list of undesirably functions could be an exported function in fs? That way we have the data in a central location. (And we could put in a little more work to recommend the specific replacements)