Comment on preceding line fine with brace_linter() (#1451)

* Comment on preceding line fine w/ `brace_linter()`

Closes #1433
Closes #1434

* Update NEWS.md

* add comment about native pipe

* remove lints

* add a failing test for regression

* start with suggested

* any prior line, not just exactly one prior

* tweak NEWS

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

Author: IndrajeetPatil

NEWS.md

@@ -6,6 +6,9 @@
 
 ## Changes to defaults
 
+* `brace_linter()` allows opening curly braces on a new line when there is 
+  a comment ending the preceding line (#1433 and #1434, @IndrajeetPatil).
+  
 * `seq_linter()` produces lint for `seq(...)`, since it also cannot properly 
   handle empty edge cases (#1468, @IndrajeetPatil).
 

R/brace_linter.R

@@ -29,10 +29,15 @@ brace_linter <- function(allow_single_line = FALSE) {
       (@line1 = following-sibling::expr/OP-LEFT-BRACE/@line1)
     )",
     # allow `(`, `,` and `%>%` on preceding line
+    #
+    # note that '{' is not supported in RHS call of base-R's native pipe (`|>`),
+    # so no exception needs to be made for this operator
     "not(
-      @line1 = parent::expr/preceding-sibling::*[1][
-        self::OP-LEFT-PAREN or self::OP-COMMA or (self::SPECIAL and text() = '%>%')
-      ]/@line2 + 1
+      @line1 > parent::expr/preceding-sibling::*[not(self::COMMENT)][1][
+        self::OP-LEFT-PAREN or
+        self::OP-COMMA or
+        (self::SPECIAL and text() = '%>%')
+      ]/@line2
     )"
   ))
 
@@ -110,41 +115,42 @@ brace_linter <- function(allow_single_line = FALSE) {
       return(list())
     }
 
+    xml <- source_expression$xml_parsed_content
     lints <- list()
 
     lints <- c(lints, xml_nodes_to_lints(
-      xml2::xml_find_all(source_expression$xml_parsed_content, xp_open_curly),
+      xml2::xml_find_all(xml, xp_open_curly),
       source_expression = source_expression,
       lint_message =
         "Opening curly braces should never go on their own line and should always be followed by a new line."
     ))
 
     lints <- c(lints, xml_nodes_to_lints(
-      xml2::xml_find_all(source_expression$xml_parsed_content, xp_paren_brace),
+      xml2::xml_find_all(xml, xp_paren_brace),
       source_expression = source_expression,
       lint_message = "There should be a space before an opening curly brace."
     ))
 
     lints <- c(lints, xml_nodes_to_lints(
-      xml2::xml_find_all(source_expression$xml_parsed_content, xp_closed_curly),
+      xml2::xml_find_all(xml, xp_closed_curly),
       source_expression = source_expression,
       lint_message = "Closing curly-braces should always be on their own line, unless they are followed by an else."
     ))
 
     lints <- c(lints, xml_nodes_to_lints(
-      xml2::xml_find_all(source_expression$xml_parsed_content, xp_else_same_line),
+      xml2::xml_find_all(xml, xp_else_same_line),
       source_expression = source_expression,
       lint_message = "`else` should come on the same line as the previous `}`."
     ))
 
     lints <- c(lints, xml_nodes_to_lints(
-      xml2::xml_find_all(source_expression$xml_parsed_content, xp_function_brace),
+      xml2::xml_find_all(xml, xp_function_brace),
       source_expression = source_expression,
       lint_message = "Any function spanning multiple lines should use curly braces."
     ))
 
     lints <- c(lints, xml_nodes_to_lints(
-      xml2::xml_find_all(source_expression$xml_parsed_content, xp_if_else_match_brace),
+      xml2::xml_find_all(xml, xp_if_else_match_brace),
       source_expression = source_expression,
       lint_message = "Either both or neither branch in `if`/`else` should use curly braces."
     ))

tests/testthat/test-brace_linter.R

@@ -100,6 +100,40 @@ test_that("brace_linter lints braces correctly", {
     linter
   )
 
+  # a comment before ,<\n>{ is allowed
+  expect_lint(
+    trim_some("
+      switch(
+        x,
+        'a' = do_something(x),
+        'b' = do_another(x), # comment
+        {
+          do_first(x)
+          do_second(x)
+        }
+      )
+    "),
+    NULL,
+    linter
+  )
+
+  # a comment before <\n>{ is allowed
+  expect_lint(
+    trim_some("
+      switch(stat,
+      o = {
+        x <- 0.01
+      },
+      # else
+      {
+        x <- 2
+      }
+    )
+    "),
+    NULL,
+    linter
+  )
+
   expect_lint(
     trim_some("
       fun(
@@ -162,6 +196,16 @@ test_that("brace_linter lints braces correctly", {
     linter
   )
 
+  expect_lint(
+    "a <- function()
+     # comment
+    {
+      1
+    }",
+    open_curly_msg,
+    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)
@@ -233,7 +277,7 @@ test_that("brace_linter lints spaces before open braces", {
     list(
       rex::rex("Opening curly braces should never go on their own line and should always be followed by a new line."),
       rex::rex("Closing curly-braces should always be on their own line, unless they are followed by an else.")
-    ), #, but not msg
+    ), # , but not msg
     linter
   )
 })