Merge branch 'main' into for-loop

Author: MichaelChirico

.gitignore

@@ -17,3 +17,4 @@ testthat-problems.rds
 
 docs
 inst/doc
+.DS_Store

.lintr

@@ -10,6 +10,7 @@ exclusions: list(
   "tests/testthat/dummy_packages",
   "tests/testthat/dummy_projects",
   "tests/testthat/exclusions-test",
+  "tests/testthat/knitr_extended_formats",
   "tests/testthat/knitr_formats",
   "tests/testthat/knitr_malformed"
  )

.lintr_new

@@ -27,6 +27,7 @@ exclusions: list(
     "tests/testthat/dummy_packages",
     "tests/testthat/dummy_projects",
     "tests/testthat/exclusions-test",
+    "tests/testthat/knitr_extended_formats",
     "tests/testthat/knitr_formats",
     "tests/testthat/knitr_malformed"
   )

DESCRIPTION

@@ -37,6 +37,7 @@ Imports:
     xml2 (>= 1.0.0),
     xmlparsedata (>= 1.0.5)
 Suggests:
+    bookdown,
     httr (>= 1.2.1),
     mockery,
     patrick,
@@ -45,6 +46,7 @@ Suggests:
     rstudioapi (>= 0.2),
     testthat (>= 3.0.0),
     tibble,
+    tufte,
     withr (>= 2.5.0)
 VignetteBuilder: 
     knitr

NEWS.md

@@ -44,7 +44,26 @@
 * `boolean_arithmetic_linter()` for identifying places where logical aggregations are more appropriate, e.g.
   `length(which(x == y)) == 0` is the same as `!any(x == y)` or even `all(x != y)` (@MichaelChirico)
 
-* `for_loop_index_linter()` to prevent overwriting local variables in a for loop declared like `for (x in x) { ... }` (@MichaelChirico)
+* `for_loop_index_linter()` to prevent overwriting local variables in a `for` loop declared like `for (x in x) { ... }` (@MichaelChirico)
+
+## Notes
+
+* `lint()` continues to support Rmarkdown documents. For users of custom .Rmd engines, e.g.
+  `marginformat` from {tufte} or `theorem` from {bookdown}, note that those engines must be registered
+  in {knitr} prior to running `lint()` in order for {lintr} to behave as expected, i.e., they should be
+  shown as part of `knitr::knit_engines$get()`.
+  
+  For {tufte} and {bookdown} in particular, one only needs to load the package namespace to accomplish
+  this (i.e., minimally `loadNamespace("tufte")` or `loadNamespace("bookdown")`, respectively, will
+  register those packages' custom engines; since `library()` also runs `loadNamespace()`, running
+  `library()` will also work). Note further that {tufte} only added this code to their `.onLoad()` recently
+  after our request to do so (see https://github.com/rstudio/tufte/issues/117). Therefore, ensure you're using a
+  more recent version to get the behavior described here for {tufte}.
+  
+  However, there is no requirement that `loadNamespace()` will register a package's custom {knitr}
+  engines, so you may need to work with other package authors to figure out a solution for other engines.
+  
+  Thanks to Yihui and other developers for their helpful discussions around this issue (#797, @IndrajeetPatil).
 
 # lintr 3.0.1
 

R/extract.R

@@ -99,6 +99,12 @@ filter_chunk_end_positions <- function(starts, ends) {
 }
 
 defines_knitr_engine <- function(start_lines) {
+  # Other packages defining custom engines should have them loaded and thus visible
+  #   via knitr_engines$get() below. It seems the simplest way to accomplish this is
+  #   for those packages to set some code in their .onLoad() hook, but that's not
+  #   always done (nor quite recommended as a "best practice" by knitr).
+  #   See the discussion on #1552.
+  # TODO(#1617): explore running loadNamespace() automatically.
   engines <- names(knitr::knit_engines$get())
 
   # {some_engine}, {some_engine label, ...} or {some_engine, ...}

R/trailing_blank_lines_linter.R

@@ -27,6 +27,7 @@ trailing_blank_lines_linter <- function() {
       }
       line_number <- line_number - 1L
     }
+
     if (identical(source_expression$terminal_newline, FALSE)) { # could use isFALSE, but needs backports
       last_line <- tail(source_expression$file_lines, 1L)
 
@@ -39,6 +40,7 @@ trailing_blank_lines_linter <- function() {
         line = last_line
       )
     }
+
     lints
   })
 }

tests/testthat.R

@@ -1,4 +1,6 @@
 library(testthat)
 library(lintr)
+loadNamespace("rex")
+loadNamespace("withr")
 
 test_check("lintr")

tests/testthat/knitr_extended_formats/bookdown.Rmd

@@ -0,0 +1,16 @@
+---
+documentclass: book
+output: bookdown::html_document2
+---
+
+# Examples
+
+```{definition}
+The characteristic function of a random variable $X$ is defined by
+$$\varphi _{X}(t)=\operatorname {E} \left[e^{itX}\right], \; t\in\mathcal{R}$$
+```
+
+```{r}
+a = 1
+```
+

tests/testthat/knitr_extended_formats/tufte.Rmd

@@ -0,0 +1,12 @@
+---
+output: tufte::tufte_html
+---
+
+```{marginfigure}
+"Hi"
+<span>- X</span>
+```
+
+```{r}
+a = 1
+```

tests/testthat/test-knitr_extended_formats.R

@@ -0,0 +1,23 @@
+test_that("marginfigure engine from tufte package doesn't cause problems", {
+  skip_if_not_installed("tufte", minimum_version = "0.12.4") # for rstudio/tufte#117
+  loadNamespace("tufte") # to register additional engines
+
+  expect_lint(
+    file = test_path("knitr_extended_formats", "tufte.Rmd"),
+    checks = list(rex::rex("Use <-, not =, for assignment."), line_number = 11L),
+    default_linters,
+    parse_settings = FALSE
+  )
+})
+
+test_that("engines from bookdown package cause no problems", {
+  skip_if_not_installed("bookdown")
+  loadNamespace("bookdown") # to register additional engines
+
+  expect_lint(
+    file = test_path("knitr_extended_formats", "bookdown.Rmd"),
+    checks = list(rex::rex("Use <-, not =, for assignment."), line_number = 14L),
+    default_linters,
+    parse_settings = FALSE
+  )
+})

tests/testthat/test-knitr_formats.R

@@ -12,82 +12,85 @@ test_that("it handles dir", {
   lints <- lint_dir(path = "knitr_formats", pattern = file_pattern, parse_settings = FALSE)
 
   # For every file there should be at least 1 lint
-  expect_identical(sort(unique(names(lints))), sort(list.files("knitr_formats", pattern = file_pattern)))
+  expect_identical(
+    sort(unique(names(lints))),
+    sort(list.files(test_path("knitr_formats"), pattern = file_pattern))
+  )
 })
 
 test_that("it handles markdown", {
   expect_lint(
-    file = "knitr_formats/test.Rmd",
+    file = test_path("knitr_formats", "test.Rmd"),
     checks = list(
       list(regexes[["assign"]], line_number = 9L),
       list(regexes[["local_var"]], line_number = 22L),
       list(regexes[["assign"]], line_number = 22L),
       list(regexes[["trailing"]], line_number = 24L)
     ),
-    default_linters,
+    linters = default_linters,
     parse_settings = FALSE
   )
 })
 
 test_that("it handles quarto", {
   expect_lint(
-    file = "knitr_formats/test.qmd",
+    file = test_path("knitr_formats", "test.qmd"),
     checks = list(
       list(regexes[["assign"]], line_number = 9L),
       list(regexes[["local_var"]], line_number = 22L),
       list(regexes[["assign"]], line_number = 22L),
       list(regexes[["trailing"]], line_number = 24L)
     ),
-    default_linters,
+    linters = default_linters,
     parse_settings = FALSE
   )
 })
 
 test_that("it handles Sweave", {
   expect_lint(
-    file = "knitr_formats/test.Rnw",
+    file = test_path("knitr_formats", "test.Rnw"),
     checks = list(
       list(regexes[["assign"]], line_number = 12L),
       list(regexes[["local_var"]], line_number = 24L),
       list(regexes[["assign"]], line_number = 24L),
       list(regexes[["trailing"]], line_number = 26L)
     ),
-    default_linters,
+    linters = default_linters,
     parse_settings = FALSE
   )
 })
 
 test_that("it handles reStructuredText", {
   expect_lint(
-    file = "knitr_formats/test.Rrst",
+    file = test_path("knitr_formats", "test.Rrst"),
     checks = list(
       list(regexes[["assign"]], line_number = 10L),
       list(regexes[["local_var"]], line_number = 23L),
       list(regexes[["assign"]], line_number = 23L),
       list(regexes[["trailing"]], line_number = 25L)
     ),
-    default_linters,
+    linters = default_linters,
     parse_settings = FALSE
   )
 })
 
 test_that("it handles HTML", {
   expect_lint(
-    file = "knitr_formats/test.Rhtml",
+    file = test_path("knitr_formats", "test.Rhtml"),
     checks = list(
       list(regexes[["assign"]], line_number = 15L),
       list(regexes[["local_var"]], line_number = 27L),
       list(regexes[["assign"]], line_number = 27L),
       list(regexes[["trailing"]], line_number = 29L)
     ),
-    default_linters,
+    linters = default_linters,
     parse_settings = FALSE
   )
 })
 
 test_that("it handles tex", {
   expect_lint(
-    file = "knitr_formats/test.Rtex",
+    file = test_path("knitr_formats", "test.Rtex"),
     checks = list(
       list(regexes[["assign"]], line_number = 11L),
       list(regexes[["local_var"]], line_number = 23L),
@@ -99,21 +102,21 @@ test_that("it handles tex", {
       # "%" as well.
       # cf. get_source_expressions("tests/testthat/knitr_formats/test.Rtex")$lines[[25]]
     ),
-    default_linters,
+    linters = default_linters,
     parse_settings = FALSE
   )
 })
 
 test_that("it handles asciidoc", {
   expect_lint(
-    file = "knitr_formats/test.Rtxt",
+    file = test_path("knitr_formats", "test.Rtxt"),
     checks = list(
       list(regexes[["assign"]], line_number = 9L),
       list(regexes[["local_var"]], line_number = 22L),
       list(regexes[["assign"]], line_number = 22L),
       list(regexes[["trailing"]], line_number = 24L)
     ),
-    default_linters,
+    linters = default_linters,
     parse_settings = FALSE
   )
 })
@@ -138,16 +141,16 @@ test_that("it does _not_ error with inline \\Sexpr", {
 
 test_that("it does lint .Rmd or .qmd file with malformed input", {
   expect_lint(
-    file = "knitr_malformed/incomplete_r_block.Rmd",
+    file = test_path("knitr_malformed", "incomplete_r_block.Rmd"),
     checks = "Missing chunk end",
-    default_linters,
+    linters = default_linters,
     parse_settings = FALSE
   )
 
   expect_lint(
-    file = "knitr_malformed/incomplete_r_block.qmd",
+    file = test_path("knitr_malformed", "incomplete_r_block.qmd"),
     checks = "Missing chunk end",
-    default_linters,
+    linters = default_linters,
     parse_settings = FALSE
   )