deprecate open_curly_linter and merge it into brace_linter

NEWS.md

@@ -11,6 +11,7 @@
 * Rename `semicolon_terminator_linter` to `semicolon_linter` for better consistency. `semicolon_terminator_linter` survives but is marked for deprecation. The new linter also has a new signature, taking arguments `allow_compound` and `allow_trailing` to replace the old single argument `semicolon=`, again for signature consistency with other linters.
 * Combined several curly brace related linters into a new `brace_linter` (#1041, @AshesITR):
   + `closed_curly_linter()`
+  + `open_curly_linter()`, no longer linting unnecessary trailing whitespace
   + Require `else` to come on the same line as the preceding `}`, if present (#884, @michaelchirico)
   + Require functions spanning multiple lines to use curly braces (@michaelchirico)
   + Require balanced usage of `{}` in `if`/`else` conditions (@michaelchirico)

R/brace_linter.R

@@ -2,7 +2,8 @@
 #'
 #' Perform various style checks related to placement and spacing of curly braces:
 #'
-#'  - Curly braces are on their own line unless they are followed by an `else`.
+#'  - Opening curly braces are never on their own line and are always followed by a newline.
+#'  - Closing curly braces are on their own line unless they are followed by an `else`.
 #'  - Closing curly braces in `if` conditions are on the same line as the corresponding `else`.
 #'  - Either both or neither branch in `if`/`else` use curly braces, i.e., either both branches use `{...}` or neither
 #'    does.
@@ -23,6 +24,36 @@ brace_linter <- function(allow_single_line = FALSE) {
 
     lints <- list()
 
+    xp_cond_open <- xp_and(c(
+      # matching } is on same line
+      if (isTRUE(allow_single_line)) {
+        "(@line1 != following-sibling::OP-LEFT-BRACE/@line1)"
+      },
+      # double curly
+      "not(
+        (@line1 = parent::expr/preceding-sibling::OP-LEFT-BRACE/@line1) or
+        (@line1 = following-sibling::expr/OP-LEFT-BRACE/@line1)
+      )"
+    ))
+
+    # TODO (AshesITR): if c_style_braces is TRUE, invert the preceding-sibling condition
+    xp_open_curly <- glue::glue("//OP-LEFT-BRACE[
+      { xp_cond_open } and (
+        not(@line1 = parent::expr/preceding-sibling::*/@line2) or
+        @line1 = following-sibling::*[1][not(self::COMMENT)]/@line1
+      )
+    ]")
+
+    lints <- c(lints, lapply(
+      xml2::xml_find_all(source_expression$xml_parsed_content, xp_open_curly),
+      xml_nodes_to_lint,
+      source_file = source_expression,
+      lint_message = paste(
+        "Opening curly braces should never go on their own line and",
+        "should always be followed by a new line."
+      )
+    ))
+
     xp_cond_closed <- xp_and(c(
       # matching { is on same line
       if (isTRUE(allow_single_line)) {

R/open_curly_linter.R

@@ -9,6 +9,7 @@
 #'   <https://style.tidyverse.org/syntax.html#indenting>
 #' @export
 open_curly_linter <- function(allow_single_line = FALSE) {
+  lintr_deprecated("open_curly_linter", new = "brace_linter", version = "2.0.1.9001", type = "Linter")
   Linter(function(source_file) {
     lapply(
       ids_with_token(source_file, "'{'"),

R/zzz.R

@@ -25,7 +25,6 @@ default_linters <- with_defaults(
   object_length_linter(),
   object_name_linter(),
   object_usage_linter(),
-  open_curly_linter(),
   paren_body_linter(),
   paren_brace_linter(),
   pipe_continuation_linter(),

inst/lintr/linters.csv

@@ -43,7 +43,7 @@ numeric_leading_zero_linter,style consistency readability
 object_length_linter,style readability default configurable
 object_name_linter,style consistency default configurable
 object_usage_linter,style readability correctness default
-open_curly_linter,style readability default configurable
+open_curly_linter,style readability configurable
 outer_negation_linter,readability efficiency best_practices
 package_hooks_linter,style correctness package_development
 paren_body_linter,style readability default

tests/testthat/test-brace_linter.R

@@ -1,4 +1,7 @@
-test_that("brace_linter lints closed braces correctly", {
+test_that("brace_linter lints braces correctly", {
+  open_curly_msg <- rex::rex(
+    "Opening curly braces should never go on their own line and should always be followed by a new line."
+  )
   closed_curly_msg <- rex::rex(paste(
     "Closing curly-braces should always be on their own line,",
     "unless they are followed by an else."
@@ -8,7 +11,7 @@ test_that("brace_linter lints closed braces correctly", {
   expect_lint("blah", NULL, linter)
   expect_lint("a <- function() {\n}", NULL, linter)
 
-  expect_lint("a <- function() { 1 }", closed_curly_msg, linter)
+  expect_lint("a <- function() { 1 }", list(open_curly_msg, closed_curly_msg), linter)
   # allowed by allow_single_line
   expect_lint("a <- function() { 1 }", NULL, brace_linter(allow_single_line = TRUE))
 
@@ -47,7 +50,7 @@ test_that("brace_linter lints closed braces correctly", {
   )
 
   # }) is allowed
-  expect_lint("eval(bquote({...}))", NULL, linter)
+  expect_lint("eval(bquote({\n...\n}))", NULL, linter)
 
   # }, is allowed
   expect_lint(
@@ -77,10 +80,40 @@ test_that("brace_linter lints closed braces correctly", {
     linter
   )
 
-  # }} is allowed
+  # {{ }} is allowed
   expect_lint("{{ x }}", NULL, linter)
+
+  expect_lint(
+    trim_some("
+      pkg_name <- function(path = find_package()) {
+        if (is.null(path)) {
+          return(NULL)
+        } else {
+          read.dcf(file.path(path, \"DESCRIPTION\"), fields = \"Package\")[1]
+        }
+      }
+    "),
+    NULL,
+    linter
+  )
+
+  expect_lint("a <- function()\n{\n  1 \n}", open_curly_msg, linter)
+  expect_lint("a <- function()\n    {\n  1 \n}", open_curly_msg, linter)
+  expect_lint("a <- function()\n\t{\n  1 \n}", open_curly_msg, linter)
+
+  # trailing comments are allowed
+  expect_lint(
+    trim_some('
+      if ("P" != "NP") { # what most people expect
+        print("Cryptomania is possible")
+      }
+    '),
+    NULL,
+    linter
+  )
 })
 
+
 test_that("brace_linter lints else correctly", {
   linter <- brace_linter()
   expect_lint("if (TRUE) 1 else 2", NULL, linter)
@@ -145,8 +178,8 @@ test_that("brace_linter lints function expressions correctly", {
   )
 })
 
-test_that("braces_linter lints if/else matching braces correctly", {
-  linter <- braces_linter()
+test_that("brace_linter lints if/else matching braces correctly", {
+  linter <- brace_linter()
   expect_lint("if (TRUE) 1 else 2", NULL, linter)
   expect_lint("if (TRUE) 1", NULL, linter)
 

tests/testthat/test-open_curly_linter.R

@@ -1,49 +1,56 @@
 test_that("returns the correct linting", {
+  msg <- rex("Opening curly braces should never go on their own line and should always be followed by a new line.")
 
-  expect_lint("blah", NULL, open_curly_linter())
+  expect_warning(
+    linter <- open_curly_linter(),
+    "Linter open_curly_linter was deprecated",
+    fixed = TRUE
+  )
+
+  expect_lint("blah", NULL, linter)
 
-  expect_lint("a <- function() {\n}", NULL, open_curly_linter())
+  expect_lint("a <- function() {\n}", NULL, linter)
 
   expect_lint(
-"pkg_name <- function(path = find_package()) {
-  if (is.null(path)) {
-    return(NULL)
-  } else {
-    read.dcf(file.path(path, \"DESCRIPTION\"), fields = \"Package\")[1]
-  }
-}", NULL, open_curly_linter())
+    "pkg_name <- function(path = find_package()) {
+      if (is.null(path)) {
+        return(NULL)
+      } else {
+        read.dcf(file.path(path, \"DESCRIPTION\"), fields = \"Package\")[1]
+      }
+    }", NULL, linter)
 
   expect_lint("a <- function()\n{\n  1 \n}",
-    rex("Opening curly braces should never go on their own line and should always be followed by a new line."),
-    open_curly_linter())
+              msg,
+              linter)
 
   expect_lint("a <- function()\n    {\n  1 \n}",
-    rex("Opening curly braces should never go on their own line and should always be followed by a new line."),
-    open_curly_linter())
+              msg,
+              linter)
 
   expect_lint("a <- function()\n\t{\n  1 \n}",
-    rex("Opening curly braces should never go on their own line and should always be followed by a new line."),
-    open_curly_linter())
+              msg,
+              linter)
 
   expect_lint("a <- function() {  \n}",
-    rex("Opening curly braces should never go on their own line and should always be followed by a new line."),
-    open_curly_linter())
+              msg,
+              linter)
 
   expect_lint("a <- function() { 1 }",
-    rex("Opening curly braces should never go on their own line and should always be followed by a new line."),
-    open_curly_linter())
+              msg,
+              linter)
 
   expect_lint("a <- function() { 1 }",
     NULL,
-    open_curly_linter(allow_single_line = TRUE))
+    suppressWarnings(open_curly_linter(allow_single_line = TRUE)))
 
   expect_lint(
 'if ("P" != "NP") { # what most people expect
     print("Cryptomania is possible")
 }',
     NULL,
-    open_curly_linter()
+linter
   )
 
-  expect_lint("{{x}}", NULL, open_curly_linter())
+  expect_lint("{{x}}", NULL, linter)
 })