From 9dc82c8fa7d0476e5e4a5d3cee4438a98eaa4309 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Thu, 23 Jan 2020 20:33:48 +0900 Subject: [PATCH 1/4] Move prohibition of data.frame to tests --- R/performance.R | 7 ------- tests/testthat/test-conditions.R | 10 ++++++++++ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/R/performance.R b/R/performance.R index 8ed0e53da3..24a20e0ae5 100644 --- a/R/performance.R +++ b/R/performance.R @@ -26,13 +26,6 @@ data_frame <- function(...) { new_data_frame(list(...)) } -data.frame <- function(...) { - abort(glue(" - Please use `data_frame()` or `new_data_frame()` instead of `data.frame()` for better performance. - See the vignette 'ggplot2 internal programming guidelines' for details. - ")) -} - split_matrix <- function(x, col_names = colnames(x)) { force(col_names) x <- lapply(seq_len(ncol(x)), function(i) x[, i]) diff --git a/tests/testthat/test-conditions.R b/tests/testthat/test-conditions.R index ed3933a6d7..24e29c404d 100644 --- a/tests/testthat/test-conditions.R +++ b/tests/testthat/test-conditions.R @@ -10,6 +10,11 @@ get_n_warning <- function(f) { sum(d$token == "SYMBOL_FUNCTION_CALL" & d$text == "warning") } +get_n_data.frame <- function(f) { + d <- getParseData(parse(f, keep.source = TRUE)) + sum(d$token == "SYMBOL_FUNCTION_CALL" & d$text == "data.frame") +} + test_that("do not use stop()", { stops <- vapply(list.files("../../R", full.names = TRUE), get_n_stop, integer(1)) expect_equal(sum(stops), 0) @@ -19,3 +24,8 @@ test_that("do not use warning()", { warnings <- vapply(list.files("../../R", full.names = TRUE), get_n_warning, integer(1)) expect_equal(sum(warnings), 0) }) + +test_that("do not use data.frame(), use `data_frame()` or `new_data_frame()`", { + data.frames <- vapply(list.files("../../R", full.names = TRUE), get_n_data.frame, integer(1)) + expect_equal(sum(data.frames), 0) +}) From 4680a49a1e69e57b66db250c5bc02ff6669f9459 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Thu, 23 Jan 2020 20:35:42 +0900 Subject: [PATCH 2/4] Rename test --- tests/testthat/{test-conditions.R => test-prohibited-functions.R} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/testthat/{test-conditions.R => test-prohibited-functions.R} (100%) diff --git a/tests/testthat/test-conditions.R b/tests/testthat/test-prohibited-functions.R similarity index 100% rename from tests/testthat/test-conditions.R rename to tests/testthat/test-prohibited-functions.R From 430fba2419a9f54db891853c96298457207b1325 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Sun, 2 Feb 2020 12:59:38 +0900 Subject: [PATCH 3/4] Check files in tests/testthat as well --- tests/testthat/test-prohibited-functions.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-prohibited-functions.R b/tests/testthat/test-prohibited-functions.R index 24e29c404d..dc801ac19c 100644 --- a/tests/testthat/test-prohibited-functions.R +++ b/tests/testthat/test-prohibited-functions.R @@ -26,6 +26,7 @@ test_that("do not use warning()", { }) test_that("do not use data.frame(), use `data_frame()` or `new_data_frame()`", { - data.frames <- vapply(list.files("../../R", full.names = TRUE), get_n_data.frame, integer(1)) + files <- list.files(c(".", "../../R"), pattern = "\\.R", full.names = TRUE) + data.frames <- vapply(files, get_n_data.frame, integer(1)) expect_equal(sum(data.frames), 0) }) From e7a78709bc446556faf243dfee5234205337ca6c Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Sun, 2 Feb 2020 13:05:18 +0900 Subject: [PATCH 4/4] Include tests about tests --- tests/testthat/test-prohibited-functions.R | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/testthat/test-prohibited-functions.R b/tests/testthat/test-prohibited-functions.R index dc801ac19c..2c5c30a5da 100644 --- a/tests/testthat/test-prohibited-functions.R +++ b/tests/testthat/test-prohibited-functions.R @@ -15,6 +15,22 @@ get_n_data.frame <- function(f) { sum(d$token == "SYMBOL_FUNCTION_CALL" & d$text == "data.frame") } +test_that("`get_n_*() detects number of calls properly", { + withr::local_file("tmp.R") + writeLines( + c( + 'stop("foo!")', + 'warning("bar!")', + "data.frame(x = 1)" + ), + "tmp.R" + ) + + expect_equal(get_n_stop("tmp.R"), 1) + expect_equal(get_n_warning("tmp.R"), 1) + expect_equal(get_n_data.frame("tmp.R"), 1) +}) + test_that("do not use stop()", { stops <- vapply(list.files("../../R", full.names = TRUE), get_n_stop, integer(1)) expect_equal(sum(stops), 0)