Skip to content

Commit 241b7c5

Browse files
Move the check on misuse of data.frame() to tests (#3758)
* Move prohibition of data.frame to tests * Rename test * Check files in tests/testthat as well * Include tests about tests
1 parent bf8786d commit 241b7c5

File tree

2 files changed

+26
-7
lines changed

2 files changed

+26
-7
lines changed

R/performance.R

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,6 @@ data_frame <- function(...) {
2626
new_data_frame(list(...))
2727
}
2828

29-
data.frame <- function(...) {
30-
abort(glue("
31-
Please use `data_frame()` or `new_data_frame()` instead of `data.frame()` for better performance.
32-
See the vignette 'ggplot2 internal programming guidelines' for details.
33-
"))
34-
}
35-
3629
split_matrix <- function(x, col_names = colnames(x)) {
3730
force(col_names)
3831
x <- lapply(seq_len(ncol(x)), function(i) x[, i])

tests/testthat/test-conditions.R renamed to tests/testthat/test-prohibited-functions.R

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,27 @@ get_n_warning <- function(f) {
1010
sum(d$token == "SYMBOL_FUNCTION_CALL" & d$text == "warning")
1111
}
1212

13+
get_n_data.frame <- function(f) {
14+
d <- getParseData(parse(f, keep.source = TRUE))
15+
sum(d$token == "SYMBOL_FUNCTION_CALL" & d$text == "data.frame")
16+
}
17+
18+
test_that("`get_n_*() detects number of calls properly", {
19+
withr::local_file("tmp.R")
20+
writeLines(
21+
c(
22+
'stop("foo!")',
23+
'warning("bar!")',
24+
"data.frame(x = 1)"
25+
),
26+
"tmp.R"
27+
)
28+
29+
expect_equal(get_n_stop("tmp.R"), 1)
30+
expect_equal(get_n_warning("tmp.R"), 1)
31+
expect_equal(get_n_data.frame("tmp.R"), 1)
32+
})
33+
1334
# Pattern is needed filter out files such as ggplot2.rdb, which is created when running covr::package_coverage()
1435
R_files <- list.files("../../R", pattern = ".*\\.(R|r)$", full.names = TRUE)
1536

@@ -22,3 +43,8 @@ test_that("do not use warning()", {
2243
warnings <- vapply(R_files, get_n_warning, integer(1))
2344
expect_equal(sum(warnings), 0)
2445
})
46+
47+
test_that("do not use data.frame(), use `data_frame()` or `new_data_frame()`", {
48+
data.frames <- vapply(R_files, get_n_data.frame, integer(1))
49+
expect_equal(sum(data.frames), 0)
50+
})

0 commit comments

Comments
 (0)