From 8bec6df3109335c560147036e2d4422e64652039 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Fri, 7 Oct 2022 08:28:23 +0200 Subject: [PATCH 1/2] Anticipate more knitr engines in code blocks (#1552) * Anticipate raw HTML from extended knitr engines Closes #796 Plus styling changes. * Also include bookdown engines #https://github.com/r-lib/lintr/issues/797 * don't piggyback; move tests to their own file * also add test for bookdown engines; ignore file * also update .lintr_new * Update test-knitr_formats.R * Update NEWS.md * don't use bookdown vector * address review comment * Update test-knitr_extended_formats.R * update comment * fix tests * Use `loadNamespace()` instead * Update for tufte update * Update NEWS.md * extend/clarify comment * qualify rex namespace, use test_path() * use test_path() * much-extended NEWS item * grammar fix Co-authored-by: Indrajeet Patil Co-authored-by: Michael Chirico --- .gitignore | 1 + .lintr | 1 + .lintr_new | 1 + DESCRIPTION | 2 ++ NEWS.md | 19 +++++++++++++++ R/extract.R | 6 +++++ .../knitr_extended_formats/bookdown.Rmd | 16 +++++++++++++ .../testthat/knitr_extended_formats/tufte.Rmd | 12 ++++++++++ tests/testthat/test-knitr_extended_formats.R | 23 +++++++++++++++++++ tests/testthat/test-knitr_formats.R | 5 +++- 10 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 tests/testthat/knitr_extended_formats/bookdown.Rmd create mode 100644 tests/testthat/knitr_extended_formats/tufte.Rmd create mode 100644 tests/testthat/test-knitr_extended_formats.R diff --git a/.gitignore b/.gitignore index 02d96231c..ee762816a 100644 --- a/.gitignore +++ b/.gitignore @@ -17,3 +17,4 @@ testthat-problems.rds docs inst/doc +.DS_Store diff --git a/.lintr b/.lintr index 8792d9421..8e6cba964 100644 --- a/.lintr +++ b/.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" ) diff --git a/.lintr_new b/.lintr_new index 47b22b5df..d1bfbe8aa 100644 --- a/.lintr_new +++ b/.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" ) diff --git a/DESCRIPTION b/DESCRIPTION index 560f5da91..0296fca65 100644 --- a/DESCRIPTION +++ b/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 diff --git a/NEWS.md b/NEWS.md index bbd4fa756..95f56f6be 100644 --- a/NEWS.md +++ b/NEWS.md @@ -44,6 +44,25 @@ * `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) +## 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 * Skip multi-byte tests in non UTF-8 locales (#1504) diff --git a/R/extract.R b/R/extract.R index 392f3d546..127088949 100644 --- a/R/extract.R +++ b/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, ...} diff --git a/tests/testthat/knitr_extended_formats/bookdown.Rmd b/tests/testthat/knitr_extended_formats/bookdown.Rmd new file mode 100644 index 000000000..49f8de66d --- /dev/null +++ b/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 +``` + diff --git a/tests/testthat/knitr_extended_formats/tufte.Rmd b/tests/testthat/knitr_extended_formats/tufte.Rmd new file mode 100644 index 000000000..3b828c419 --- /dev/null +++ b/tests/testthat/knitr_extended_formats/tufte.Rmd @@ -0,0 +1,12 @@ +--- +output: tufte::tufte_html +--- + +```{marginfigure} +"Hi" +- X +``` + +```{r} +a = 1 +``` diff --git a/tests/testthat/test-knitr_extended_formats.R b/tests/testthat/test-knitr_extended_formats.R new file mode 100644 index 000000000..35dcbd652 --- /dev/null +++ b/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 + ) +}) diff --git a/tests/testthat/test-knitr_formats.R b/tests/testthat/test-knitr_formats.R index 027b70f6f..c7a9ed747 100644 --- a/tests/testthat/test-knitr_formats.R +++ b/tests/testthat/test-knitr_formats.R @@ -12,7 +12,10 @@ 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", { From a6254c2cebe2b46abf67a79d4d443383aa6d8c4d Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Fri, 7 Oct 2022 08:52:30 +0200 Subject: [PATCH 2/2] Tests for `trailing_blank_lines_linter()` with chunks (#1614) * Tests for `trailing_blank_lines_linter()` with chunks Closes #741 * Use `library(withr)` * formatting * restructure * Don't set `content` to `NULL` * make it DAMP * restructure to bring message closer to expectations * use namespace * use loadNamespace() for test-required packages * Update test-trailing_blank_lines_linter.R * Apply cleanup to the rest of the file * same clean up in another test file * Use `trim_some()` Co-authored-by: Michael Chirico --- R/trailing_blank_lines_linter.R | 2 + tests/testthat.R | 2 + tests/testthat/test-knitr_formats.R | 36 ++--- .../test-trailing_blank_lines_linter.R | 134 ++++++++++++++---- 4 files changed, 125 insertions(+), 49 deletions(-) diff --git a/R/trailing_blank_lines_linter.R b/R/trailing_blank_lines_linter.R index 5148cc090..65b3da363 100644 --- a/R/trailing_blank_lines_linter.R +++ b/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 }) } diff --git a/tests/testthat.R b/tests/testthat.R index a0ff76ddb..0c92bc974 100644 --- a/tests/testthat.R +++ b/tests/testthat.R @@ -1,4 +1,6 @@ library(testthat) library(lintr) +loadNamespace("rex") +loadNamespace("withr") test_check("lintr") diff --git a/tests/testthat/test-knitr_formats.R b/tests/testthat/test-knitr_formats.R index c7a9ed747..a6899e074 100644 --- a/tests/testthat/test-knitr_formats.R +++ b/tests/testthat/test-knitr_formats.R @@ -20,77 +20,77 @@ test_that("it handles dir", { 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), @@ -102,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 ) }) @@ -141,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 ) diff --git a/tests/testthat/test-trailing_blank_lines_linter.R b/tests/testthat/test-trailing_blank_lines_linter.R index d48c94319..7f07f714a 100644 --- a/tests/testthat/test-trailing_blank_lines_linter.R +++ b/tests/testthat/test-trailing_blank_lines_linter.R @@ -1,35 +1,43 @@ -test_that("returns the correct linting", { +test_that("trailing_blank_lines_linter doesn't block allowed usages", { linter <- trailing_blank_lines_linter() - msg <- rex::rex("Trailing blank lines are superfluous.") - msg2 <- rex::rex("Missing terminal newline.") expect_lint("blah", NULL, linter) expect_lint("blah <- 1 ", NULL, linter) expect_lint("blah <- 1\nblah", NULL, linter) expect_lint("blah <- 1\nblah\n \n blah", NULL, linter) + tmp <- withr::local_tempfile(lines = "lm(y ~ x)") + expect_lint(file = tmp, checks = NULL, linters = linter) +}) + +test_that("trailing_blank_lines_linter detects disallowed usages", { + linter <- trailing_blank_lines_linter() + msg <- rex::rex("Trailing blank lines are superfluous.") + expect_lint("blah <- 1\n", msg, linter) expect_lint("blah <- 1\n ", msg, linter) - expect_lint("blah <- 1\n \n ", list(msg, msg), linter) expect_lint("blah <- 1\n\n", list(msg, msg), linter) expect_lint("blah <- 1\n\t\n", list(msg, msg), linter) # Construct a test file without terminal newline # cf. test-get_source_expressions.R - tmp <- withr::local_tempfile() tmp2 <- withr::local_tempfile() - cat("lm(y ~ x)\n", file = tmp) cat("lm(y ~ x)", file = tmp2) + expect_lint( + file = tmp2, + checks = list( + message = rex::rex("Missing terminal newline."), + line_number = 1L, + column_number = 10L + ), + linters = linter + ) +}) - expect_lint(content = NULL, file = tmp, NULL, linter) - expect_lint(content = NULL, file = tmp2, list( - message = msg2, - line_number = 1L, - column_number = 10L - ), linter) +test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/qmd docs", { + linter <- trailing_blank_lines_linter() - # Construct an Rmd file without terminal newline tmp3 <- withr::local_tempfile(fileext = ".Rmd") cat( trim_some( @@ -46,12 +54,16 @@ test_that("returns the correct linting", { ), file = tmp3 ) - expect_lint(content = NULL, file = tmp3, list( - message = msg2, - line_number = 10L, - # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. - column_number = 1L - ), linter) + expect_lint( + file = tmp3, + checks = list( + message = rex::rex("Missing terminal newline."), + line_number = 10L, + # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. + column_number = 1L + ), + linters = linter + ) # Construct an Rmd file without R code (#1415) tmp4 <- withr::local_tempfile(fileext = ".Rmd") @@ -65,12 +77,16 @@ test_that("returns the correct linting", { ), file = tmp4 ) - expect_lint(content = NULL, file = tmp4, list( - message = msg2, - line_number = 5L, - # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. - column_number = 1L - ), linter) + expect_lint( + file = tmp4, + checks = list( + message = rex::rex("Missing terminal newline."), + line_number = 5L, + # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. + column_number = 1L + ), + linters = linter + ) # Construct a qmd file without terminal newline tmp5 <- withr::local_tempfile(fileext = ".qmd") @@ -89,10 +105,66 @@ test_that("returns the correct linting", { ), file = tmp5 ) - expect_lint(content = NULL, file = tmp5, list( - message = msg2, - line_number = 10L, - # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. - column_number = 1L - ), linter) + expect_lint( + file = tmp5, + checks = list( + message = rex::rex("Missing terminal newline."), + line_number = 10L, + # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. + column_number = 1L + ), + linters = linter + ) +}) + +test_that("blank lines in knitr chunks produce lints", { + linter <- trailing_blank_lines_linter() + + tmp6 <- withr::local_tempfile( + fileext = ".Rmd", + lines = trim_some( + '--- + title: "Some file" + --- + + ```{r} + abc = 123 + + ``` + \n' + ) + ) + + expect_lint( + file = tmp6, + checks = list(message = rex::rex("Trailing blank lines are superfluous."), line_number = 7L, column_number = 1L), + linters = linter + ) + + tmp7 <- withr::local_tempfile( + fileext = ".qmd", + lines = trim_some( + '--- + title: "Some file" + --- + + ```{r} + abc = 123 + + + + ``` + \n' + ) + ) + + expect_lint( + file = tmp7, + checks = list( + list(message = rex::rex("Trailing blank lines are superfluous."), line_number = 7L, column_number = 1L), + list(message = rex::rex("Trailing blank lines are superfluous."), line_number = 8L, column_number = 1L), + list(message = rex::rex("Trailing blank lines are superfluous."), line_number = 9L, column_number = 1L) + ), + linters = linter + ) })