Deprecate extraction_operator_linter() (#2409)

* Deprecate `extraction_operator_linter()` closes #1485 * Update NEWS.md * move it to deprecated linter file and re-document * Update NEWS.md Co-authored-by: Michael Chirico <[email protected]> * Update NEWS.md * Remove extraction_operator_linter from .lintr --------- Co-authored-by: Michael Chirico <[email protected]>
r-lib · Dec 10, 2023 · 32cb18c · 32cb18c
1 parent 3fd90ba
commit 32cb18c
Show file tree

Hide file tree

Showing 13 changed files with 58 additions and 150 deletions.
diff --git a/.lintr b/.lintr
@@ -14,7 +14,6 @@ linters: all_linters(
     )),
     unnecessary_concatenation_linter(allow_single_expression = FALSE),
     absolute_path_linter = NULL,
-    extraction_operator_linter = NULL,
     library_call_linter = NULL,
     nonportable_path_linter = NULL,
     todo_comment_linter = NULL,

diff --git a/DESCRIPTION b/DESCRIPTION
@@ -101,7 +101,6 @@ Collate:
     'expect_true_false_linter.R'
     'expect_type_linter.R'
     'extract.R'
-    'extraction_operator_linter.R'
     'fixed_regex_linter.R'
     'for_loop_index_linter.R'
     'function_argument_linter.R'

diff --git a/NEWS.md b/NEWS.md
@@ -12,6 +12,7 @@
    + Helper `with_defaults()`.
 * `all_linters()` has signature `all_linters(..., packages)` rather than `all_linters(packages, ...)` (#2332, @MichaelChirico). This forces `packages=` to be supplied by name and will break users who rely on supplying `packages=` positionally, of which we found none searching GitHub.
 * Adjusted various lint messages for consistency in readability (#1330, @MichaelChirico). In general, we favor lint messages to be phrased like "Action, reason" to but the "what" piece of the message front-and-center. This may be a breaking change for code that tests the specific phrasing of lints.
+* `extraction_operator_linter()` is deprecated. Although switching from `$` to `[[` has some robustness benefits for package code, it can lead to non-idiomatic code in many contexts (e.g. R6 classes, Shiny applications, etc.) (#2409, @IndrajeetPatil). To enable the detection of the `$` operator for extraction through partial matching, use `options(warnPartialMatchDollar = TRUE)`.
 
 ## Bug fixes
 

diff --git a/R/extraction_operator_linter.R b/R/extraction_operator_linter.R
diff --git a/R/lintr-deprecated.R b/R/lintr-deprecated.R
@@ -125,3 +125,39 @@ no_tab_linter <- function() {
   )
   whitespace_linter()
 }
+
+#' Extraction operator linter
+#' @rdname lintr-deprecated
+#' @export
+extraction_operator_linter <- function() {
+  lintr_deprecated(
+    what = "extraction_operator_linter",
+    version = "3.2.0",
+    type = "Linter",
+    signal = "warning"
+  )
+
+  constant_nodes_in_brackets <- paste0("self::", c("expr", "OP-PLUS", "NUM_CONST", "STR_CONST"))
+  xpath <- glue("
+  //OP-DOLLAR[not(preceding-sibling::expr[1]/SYMBOL[text() = 'self' or text() = '.self'])]
+  |
+  //OP-LEFT-BRACKET[
+    not(following-sibling::expr[1]/descendant::*[not({xp_or(constant_nodes_in_brackets)})]) and
+    not(following-sibling::OP-COMMA)
+  ]
+  ")
+
+  Linter(linter_level = "expression", function(source_expression) {
+    xml <- source_expression$xml_parsed_content
+
+    bad_exprs <- xml_find_all(xml, xpath)
+    msgs <- sprintf("Use `[[` instead of `%s` to extract an element.", xml_text(bad_exprs))
+
+    xml_nodes_to_lints(
+      bad_exprs,
+      source_expression = source_expression,
+      lint_message = msgs,
+      type = "warning"
+    )
+  })
+}
diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv
@@ -31,7 +31,7 @@ expect_s3_class_linter,package_development best_practices pkg_testthat
 expect_s4_class_linter,package_development best_practices pkg_testthat
 expect_true_false_linter,package_development best_practices readability pkg_testthat
 expect_type_linter,package_development best_practices pkg_testthat
-extraction_operator_linter,style best_practices
+extraction_operator_linter,style best_practices deprecated
 fixed_regex_linter,best_practices readability efficiency configurable regex
 for_loop_index_linter,best_practices readability robustness
 function_argument_linter,style consistency best_practices

diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd
diff --git a/man/deprecated_linters.Rd b/man/deprecated_linters.Rd
diff --git a/man/extraction_operator_linter.Rd b/man/extraction_operator_linter.Rd
diff --git a/man/linters.Rd b/man/linters.Rd
diff --git a/man/lintr-deprecated.Rd b/man/lintr-deprecated.Rd
diff --git a/man/style_linters.Rd b/man/style_linters.Rd
diff --git a/tests/testthat/test-extraction_operator_linter.R b/tests/testthat/test-extraction_operator_linter.R
@@ -1,5 +1,14 @@
+test_that("extraction_operator_linter generates deprecation warning", {
+  expect_warning(
+    extraction_operator_linter(),
+    rex::rex("Linter extraction_operator_linter was deprecated")
+  )
+})
+
 test_that("extraction_operator_linter skips allowed usages", {
-  linter <- extraction_operator_linter()
+  expect_warning({
+    linter <- extraction_operator_linter()
+  })
 
   expect_lint("x[[1]]", NULL, linter)
   expect_lint("x[-1]", NULL, linter)
@@ -10,7 +19,9 @@ test_that("extraction_operator_linter skips allowed usages", {
 })
 
 test_that("extraction_operator_linter blocks disallowed usages", {
-  linter <- extraction_operator_linter()
+  expect_warning({
+    linter <- extraction_operator_linter()
+  })
   msg_b <- rex::escape("Use `[[` instead of `[` to extract an element.")
   msg_d <- rex::escape("Use `[[` instead of `$` to extract an element.")