From b03b4293a7d74a5d2f9e77405be96b6c79b65f3a Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 23 Oct 2022 09:01:46 +0200 Subject: [PATCH 1/5] Further improvements for `lengths_linter()` Closes #1573 --- R/lengths_linter.R | 2 +- tests/testthat/test-lengths_linter.R | 37 +++++++++++++--------------- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/R/lengths_linter.R b/R/lengths_linter.R index 74701b695a..b29bdf57c5 100644 --- a/R/lengths_linter.R +++ b/R/lengths_linter.R @@ -34,7 +34,7 @@ lengths_linter <- function() { loop_funs <- c("sapply", "vapply", "map_int", "map_dbl") xpath <- glue::glue(" //SYMBOL_FUNCTION_CALL[ {xp_text_in_table(loop_funs)} ] - /parent::expr/parent::expr[expr[position() = 3 and SYMBOL[text() = 'length']]] + /parent::expr/parent::expr[expr[SYMBOL[text() = 'length']]] ") Linter(function(source_expression) { diff --git a/tests/testthat/test-lengths_linter.R b/tests/testthat/test-lengths_linter.R index a69684be30..e088902b6b 100644 --- a/tests/testthat/test-lengths_linter.R +++ b/tests/testthat/test-lengths_linter.R @@ -4,29 +4,26 @@ test_that("lengths_linter skips allowed usages", { }) test_that("lengths_linter blocks simple disallowed base usages", { - expect_lint( - "sapply(x, length)", - rex::rex("Use lengths() to find the length of each element in a list."), - lengths_linter() - ) + linter <- lengths_linter() + lint_msg <- rex::rex("Use lengths() to find the length of each element in a list.") - expect_lint( - "vapply(x, length, integer(1L))", - rex::rex("Use lengths() to find the length of each element in a list."), - lengths_linter() - ) + expect_lint("sapply(x, length)", lint_msg, linter) + expect_lint("sapply(x, FUN = length)", lint_msg, linter) + expect_lint("sapply(FUN = length, x)", lint_msg, linter) + expect_lint("x |> sapply(length)", lint_msg, linter) + expect_lint("x %>% sapply(length)", lint_msg, linter) + + expect_lint("vapply(x, length, integer(1L))", lint_msg, linter) }) test_that("lengths_linter blocks simple disallowed purrr usages", { - expect_lint( - "purrr::map_dbl(x, length)", - rex::rex("Use lengths() to find the length of each element in a list."), - lengths_linter() - ) + linter <- lengths_linter() + lint_msg <- rex::rex("Use lengths() to find the length of each element in a list.") - expect_lint( - "map_int(x, length)", - rex::rex("Use lengths() to find the length of each element in a list."), - lengths_linter() - ) + expect_lint("purrr::map_dbl(x, length)", lint_msg, linter) + expect_lint("map_dbl(x, .f = length)", lint_msg, linter) + expect_lint("map_dbl(.f = length, x)", lint_msg, linter) + expect_lint("map_int(x, length)", lint_msg, linter) + expect_lint("x |> map_int(length)", lint_msg, linter) + expect_lint("x %>% map_int(length)", lint_msg, linter) }) From b2a36f7ecc3cb5d105fc24ae4877140ed5c78094 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 23 Oct 2022 09:11:56 +0200 Subject: [PATCH 2/5] Update test-lengths_linter.R --- tests/testthat/test-lengths_linter.R | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/testthat/test-lengths_linter.R b/tests/testthat/test-lengths_linter.R index e088902b6b..e797bbe9d0 100644 --- a/tests/testthat/test-lengths_linter.R +++ b/tests/testthat/test-lengths_linter.R @@ -1,6 +1,11 @@ test_that("lengths_linter skips allowed usages", { + linter <- lengths_linter() + + expect_lint("length(x)", NULL, linter) + expect_lint("function(x) length(x) + 1L", NULL, linter) + # TODO(#1570): also throw a lint here, and for map(x, length) - expect_lint("lapply(x, length)", NULL, lengths_linter()) + expect_lint("lapply(x, length)", NULL, linter) }) test_that("lengths_linter blocks simple disallowed base usages", { @@ -10,8 +15,6 @@ test_that("lengths_linter blocks simple disallowed base usages", { expect_lint("sapply(x, length)", lint_msg, linter) expect_lint("sapply(x, FUN = length)", lint_msg, linter) expect_lint("sapply(FUN = length, x)", lint_msg, linter) - expect_lint("x |> sapply(length)", lint_msg, linter) - expect_lint("x %>% sapply(length)", lint_msg, linter) expect_lint("vapply(x, length, integer(1L))", lint_msg, linter) }) @@ -24,6 +27,17 @@ test_that("lengths_linter blocks simple disallowed purrr usages", { expect_lint("map_dbl(x, .f = length)", lint_msg, linter) expect_lint("map_dbl(.f = length, x)", lint_msg, linter) expect_lint("map_int(x, length)", lint_msg, linter) +}) + +test_that("lengths_linter blocks simple disallowed usages with pipes", { + skip_if_not_r_version("4.1.0") + + linter <- lengths_linter() + lint_msg <- rex::rex("Use lengths() to find the length of each element in a list.") + + expect_lint("x |> sapply(length)", lint_msg, linter) + expect_lint("x %>% sapply(length)", lint_msg, linter) + expect_lint("x |> map_int(length)", lint_msg, linter) expect_lint("x %>% map_int(length)", lint_msg, linter) }) From 8443518dba03c0a812ee7c2943e986c6efd82336 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Mon, 24 Oct 2022 08:59:59 +0200 Subject: [PATCH 3/5] Update test-lengths_linter.R --- tests/testthat/test-lengths_linter.R | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testthat/test-lengths_linter.R b/tests/testthat/test-lengths_linter.R index e797bbe9d0..9ebcc0fca3 100644 --- a/tests/testthat/test-lengths_linter.R +++ b/tests/testthat/test-lengths_linter.R @@ -3,6 +3,7 @@ test_that("lengths_linter skips allowed usages", { expect_lint("length(x)", NULL, linter) expect_lint("function(x) length(x) + 1L", NULL, linter) + expect_lint("vapply(x, fun, integer(length(y)))", NULL, linter) # TODO(#1570): also throw a lint here, and for map(x, length) expect_lint("lapply(x, length)", NULL, linter) From 907873ed2d85a5ce2c0e058505a96db02b060cbf Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 27 Oct 2022 08:38:00 +0200 Subject: [PATCH 4/5] Update test-lengths_linter.R --- tests/testthat/test-lengths_linter.R | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testthat/test-lengths_linter.R b/tests/testthat/test-lengths_linter.R index 9ebcc0fca3..6392698e33 100644 --- a/tests/testthat/test-lengths_linter.R +++ b/tests/testthat/test-lengths_linter.R @@ -4,6 +4,7 @@ test_that("lengths_linter skips allowed usages", { expect_lint("length(x)", NULL, linter) expect_lint("function(x) length(x) + 1L", NULL, linter) expect_lint("vapply(x, fun, integer(length(y)))", NULL, linter) + expect_lint("sapply(x, sqrt, simplify = length(x))", NULL, linter) # TODO(#1570): also throw a lint here, and for map(x, length) expect_lint("lapply(x, length)", NULL, linter) From f999174f4f6404a59a8f3923eb95a9ff418926dd Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 26 Oct 2022 23:40:23 -0700 Subject: [PATCH 5/5] remove one [] pair, bump one axis call to new line --- R/lengths_linter.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/R/lengths_linter.R b/R/lengths_linter.R index b29bdf57c5..5f37bc3f2a 100644 --- a/R/lengths_linter.R +++ b/R/lengths_linter.R @@ -34,7 +34,8 @@ lengths_linter <- function() { loop_funs <- c("sapply", "vapply", "map_int", "map_dbl") xpath <- glue::glue(" //SYMBOL_FUNCTION_CALL[ {xp_text_in_table(loop_funs)} ] - /parent::expr/parent::expr[expr[SYMBOL[text() = 'length']]] + /parent::expr + /parent::expr[expr/SYMBOL[text() = 'length']] ") Linter(function(source_expression) {