Merge pull request #801 from jimhester/pipe-call

New pipe_call_linter enforcing calls (not symbols) in magrittr pipes

Author: AshesITR

DESCRIPTION

@@ -78,6 +78,7 @@ Collate:
     'open_curly_linter.R'
     'paren_brace_linter.R'
     'path_linters.R'
+    'pipe_call_linter.R'
     'pipe_continuation_linter.R'
     'semicolon_terminator_linter.R'
     'seq_linter.R'

NEWS.md

@@ -63,6 +63,7 @@
 * `lint_package()` warns and returns `NULL` if no package is found (instead of giving a peculiar error message) (#776, @michaelchirico)
 * `lint_package()` is also stricter about what it considers to be a package -- folders named `DESCRIPTION` are ignored (#702, @michaelchirico)
 * `lint()` now has a new optional argument `text` for supplying a string or lines directly, e.g. if the file is already in memory or linting is being done ad hoc. (#503, @renkun-ken)
+* New `pipe_call_linter()` enforces that all steps of `magrittr` pipelines use explicit calls instead of symbols, e.g. `x %>% mean()` instead of `x %>% mean` (@michaelchirico)
 
 # lintr 2.0.1
 

R/pipe_call_linter.R

@@ -0,0 +1,25 @@
+#' @describeIn linters that forces explicit calls in magrittr pipes
+#' @export
+pipe_call_linter <- function() {
+  Linter(function(source_file) {
+    if (length(source_file$parsed_content) == 0L) {
+      return(list())
+    }
+
+    xml <- source_file$xml_parsed_content
+
+    # NB: the text() here shows up as %&gt;% but that's not matched, %>% is
+    # NB: use *[1][self::SYMBOL] to ensure the first element is SYMBOL, otherwise
+    #       we include expressions like x %>% .$col
+    xpath <- "//expr[preceding-sibling::SPECIAL[text() = '%>%'] and *[1][self::SYMBOL]]"
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+
+    return(lapply(
+      bad_expr,
+      xml_nodes_to_lint,
+      source_file = source_file,
+      message = "Use explicit calls in magrittr pipes, i.e., `a %>% foo` should be `a %>% foo()`.",
+      type = "warning"
+    ))
+  })
+}

tests/testthat/test-pipe_call_linter.R

@@ -0,0 +1,58 @@
+test_that("pipe_call_linter skips allowed usages", {
+  expect_lint("a %>% foo()", NULL, pipe_call_linter)
+  expect_lint("a %>% foo(x)", NULL, pipe_call_linter)
+  expect_lint("b %>% { foo(., ., .) }", NULL, pipe_call_linter)
+  expect_lint("a %>% foo() %>% bar()", NULL, pipe_call_linter)
+
+  # ensure it works across lines too
+  lines <- trim_some("
+  a %>%
+    foo() %>%
+    bar()
+  ")
+  expect_lint(lines, NULL, pipe_call_linter)
+
+  # symbol extraction is OK (don't force extract2(), e.g.)
+  expect_lint("a %>% .$y %>% mean()", NULL, pipe_call_linter)
+  
+  # more complicated expressions don't pick up on nested symbols
+  lines <- trim_some("
+  x %>% {
+    tmp <- .
+    bla <- foo %>% unrelated_stuff(tmp)
+    my_combination_fun(tmp, bla)
+  }
+  ")
+  expect_lint(lines, NULL, pipe_call_linter)
+})
+
+test_that("pipe_call_linter blocks simple disallowed usages", {
+  expect_lint(
+    "x %>% foo",
+    "Use explicit calls in magrittr pipes",
+    pipe_call_linter
+  )
+
+  expect_lint(
+    "x %>% foo() %>% bar",
+    "Use explicit calls in magrittr pipes",
+    pipe_call_linter
+  )
+
+  expect_lint(
+    "x %>% foo %>% bar()",
+    "Use explicit calls in magrittr pipes",
+    pipe_call_linter
+  )
+
+  lines <- trim_some("
+  a %>%
+    foo %>%
+    bar()
+  ")
+  expect_lint(
+    lines,
+    "Use explicit calls in magrittr pipes",
+    pipe_call_linter
+  )
+})