Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anticipate more knitr engines in code blocks #1552

Merged
merged 38 commits into from
Oct 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
4d7b0a8
Anticipate raw HTML from extended knitr engines
IndrajeetPatil Sep 24, 2022
96497bc
Also include bookdown engines
IndrajeetPatil Sep 24, 2022
ca1580d
don't piggyback; move tests to their own file
IndrajeetPatil Sep 24, 2022
1665e3d
also add test for bookdown engines; ignore file
IndrajeetPatil Sep 24, 2022
cc55556
also update .lintr_new
IndrajeetPatil Sep 24, 2022
61e6864
Update test-knitr_formats.R
IndrajeetPatil Sep 24, 2022
c617762
Update NEWS.md
IndrajeetPatil Sep 24, 2022
1e9f4b7
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 24, 2022
f27fb13
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 24, 2022
9a5a380
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 25, 2022
2fd0e54
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 25, 2022
d6de1eb
don't use bookdown vector
IndrajeetPatil Sep 25, 2022
17455b8
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 25, 2022
16de9df
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 25, 2022
fb79a0b
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 26, 2022
539335a
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 26, 2022
631d98c
address review comment
IndrajeetPatil Sep 26, 2022
cba39a1
Update test-knitr_extended_formats.R
IndrajeetPatil Sep 26, 2022
d374480
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 27, 2022
f983f2f
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 28, 2022
e896bb3
update comment
IndrajeetPatil Sep 28, 2022
ca66e97
fix tests
IndrajeetPatil Sep 28, 2022
1dedc78
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 28, 2022
49c35b6
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 29, 2022
ac9a698
Merge branch 'main' into 796_tufte
IndrajeetPatil Sep 30, 2022
26bcf1d
Use `loadNamespace()` instead
IndrajeetPatil Oct 1, 2022
94c50f3
Merge branch 'main' into 796_tufte
IndrajeetPatil Oct 1, 2022
bf42e61
Merge branch 'main' into 796_tufte
IndrajeetPatil Oct 2, 2022
c69c387
Merge branch 'main' into 796_tufte
IndrajeetPatil Oct 3, 2022
e16f589
Update for tufte update
IndrajeetPatil Oct 3, 2022
44d649b
Update NEWS.md
IndrajeetPatil Oct 4, 2022
a0f6741
Merge branch 'main' into 796_tufte
IndrajeetPatil Oct 5, 2022
c47975f
Merge branch 'main' into 796_tufte
MichaelChirico Oct 7, 2022
89bafe5
extend/clarify comment
MichaelChirico Oct 7, 2022
aaa1604
qualify rex namespace, use test_path()
MichaelChirico Oct 7, 2022
933813a
use test_path()
MichaelChirico Oct 7, 2022
d4f056b
much-extended NEWS item
MichaelChirico Oct 7, 2022
70d852c
grammar fix
MichaelChirico Oct 7, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ testthat-problems.rds

docs
inst/doc
.DS_Store
1 change: 1 addition & 0 deletions .lintr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ exclusions: list(
"tests/testthat/dummy_packages",
"tests/testthat/dummy_projects",
"tests/testthat/exclusions-test",
"tests/testthat/knitr_extended_formats",
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
"tests/testthat/knitr_formats",
"tests/testthat/knitr_malformed"
)
1 change: 1 addition & 0 deletions .lintr_new
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
2 changes: 2 additions & 0 deletions DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ Imports:
xml2 (>= 1.0.0),
xmlparsedata (>= 1.0.5)
Suggests:
bookdown,
IndrajeetPatil marked this conversation as resolved.
Show resolved Hide resolved
httr (>= 1.2.1),
mockery,
patrick,
Expand All @@ -45,6 +46,7 @@ Suggests:
rstudioapi (>= 0.2),
testthat (>= 3.0.0),
tibble,
tufte,
withr (>= 2.5.0)
VignetteBuilder:
knitr
Expand Down
19 changes: 19 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IndrajeetPatil PTAL. I moved it from "bug fixes" because there's no longer any code change by lintr. I still don't feel like I have a 100% grasp of the situation, so feel free to refine the item further.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Michael. This looks good to me.

I think we can revise this further as our understanding of the issue evolves while working on #1617.

`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)
Expand Down
6 changes: 6 additions & 0 deletions R/extract.R
Original file line number Diff line number Diff line change
Expand Up @@ -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, ...}
Expand Down
16 changes: 16 additions & 0 deletions tests/testthat/knitr_extended_formats/bookdown.Rmd
Original file line number Diff line number Diff line change
@@ -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
```

12 changes: 12 additions & 0 deletions tests/testthat/knitr_extended_formats/tufte.Rmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
output: tufte::tufte_html
---

```{marginfigure}
"Hi"
<span>- X</span>
```

```{r}
a = 1
```
23 changes: 23 additions & 0 deletions tests/testthat/test-knitr_extended_formats.R
Original file line number Diff line number Diff line change
@@ -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
)
})
5 changes: 4 additions & 1 deletion tests/testthat/test-knitr_formats.R
Original file line number Diff line number Diff line change
Expand Up @@ -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", {
Expand Down