code linter for timezone argument
Closed this issue · 0 comments
I recently got questions from a user of the in-development AirSensor package about timezone shifting. In reviewing our code I realized, to my horror, that the code is inconsistent in passing the timezone argument to functions that accept it.
Code developed at Mazama Science needs to always, always, ALWAYS specify timezones! Given that I invariably hire inexperienced R programmers, the burden for this guarantee falls on me. So let's automate this. I reviewed the lintr package and decided that it is too heavyweight.
The top priority is to have something quickly that I can at least use by hand to test code. This will include:
-
findFunctionCalls()
-
test_usesArgument()
for a single source file - enhanced
test_usesArgument()
where we can specify a directory and a set of file extensions and (recursive = TRUE?) and have it test every file
I came up with the following barely working example for what I want:
test_usesArgument_B <- function(
packageFUNC = NULL,
functionName = NULL,
argumentName = NULL
) {
argumentPattern <- paste0(argumentName,"=|",argumentName,"\\s=")
functionString <- paste0( as.character(rlang::fn_body(packageFUNC)), collapse="\n")
testResult <- stringr::str_detect(findFunctionCalls(functionString, functionName), argumentPattern)
# Could optionally spit out offending calls
return(testResult)
}
test_usesArgument <- function(
file = NULL,
functionName = NULL,
argumentName = NULL
) {
argumentPattern <- paste0(argumentName,"=|",argumentName,"\\s=")
functionString <- readr::read_file(file)
testResult <- stringr::str_detect(findFunctionCalls(functionString, functionName), argumentPattern)
# Could optionally spit out offending calls
return(testResult)
}
findFunctionCalls <- function(
functionString = NULL,
functionName = NULL
) {
functionOpen <- paste0(functionName, "\\(")
openParens <- stringr::str_locate_all(functionString, "\\(")[[1]][,1]
closeParens <- stringr::str_locate_all(functionString, "\\)")[[1]][,1]
# TODO: A better regex above would avoid matching escaped parens so that
# TODO: we wouldn't need the check below
if ( length(openParens) != length(closeParens) )
stop("Unmatched parens (Are you using strings with single escaped parens?)")
functionStarts <- stringr::str_locate_all(functionString, functionOpen)[[1]][,1]
functionEnds <- stringr::str_locate_all(functionString, functionOpen)[[1]][,2]
indices <- which(openParens %in% (functionEnds))
starts <- functionStarts
ends <- closeParens[indices]
functionCallStrings <- stringr::str_sub(functionString, starts, ends)
return(functionCallStrings)
}
# ===== DEBUGGING ==============================================================
if ( FALSE ) {
file <- "R/pas_load.R"
functionName <- "strftime"
# Test on source code in the AirSensor package
stringr::str_detect(findFunctionCalls(readr::read_file(file), "strftime"), "tz=|tz\\s=")
#[1] TRUE TRUE TRUE TRUE
all(stringr::str_detect(findFunctionCalls(readr::read_file(file), "strftime"), "tz=|tz\\s="))
#[1] TRUE
test_usesArgument("R/pas_load.R", "strftime", "tz")
#[1] TRUE TRUE TRUE TRUE
test_usesArgument_B(PWFSLSmoke::monitor_dailyBarplot, "strftime", "tz")
#[1] TRUE TRUE
}
Ideally, we could include a test inside of a package that looked something like this:
context("test-timezone-usage")
test_that("timezone is always specified", {
expect_true( all( MazamaCoreUtils::test_usesArgument(<package source>, "strftime", "timezone") )
})
expect_true( all( MazamaCoreUtils::test_usesArgument(<package source>, "strptime", "timezone") )
})
... ditto for "lubridate::ymd", etc. etc. ...
Here is what I am already doing with the prototype functionality:
files <- list.files("R", full.names = TRUE)
for ( file in files ) {
result <- try({
test_usesArgument(readr::read_file(file), "strftime", "tz")
}, silent = TRUE)
if ( "try-error" %in% class(result) ) {
cat(file)
cat("\n")
cat(geterrmessage())
cat("\n\n")
} else if ( length(result) > 0 && !any(result) ) {
cat(file)
cat("\n")
cat(result)
cat("\n\n")
}
}