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.

Sign up for GitHub

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

Add knit_cnd_format() generic #2078

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add knit_cnd_format() generic #2078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a77add
Add `knit_cnd_format()` generic
lionel- Dec 2, 2021
2e37579
Collapse messages collected by `merge_class()`
lionel- Dec 2, 2021
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
3 changes: 2 additions & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ Suggests:
bslib,
ragg,
styler (>= 1.2.0),
targets (>= 0.6.0)
targets (>= 0.6.0),
rlang
License: GPL
URL: https://yihui.org/knitr/
BugReports: https://github.com/yihui/knitr/issues
Expand Down
4 changes: 4 additions & 0 deletions NAMESPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

S3method("$",knitr_strict_list)
S3method(is_low_change,default)
S3method(knit_cnd_format,error)
S3method(knit_cnd_format,message)
S3method(knit_cnd_format,warning)
S3method(knit_print,default)
S3method(knit_print,knit_asis)
S3method(knit_print,knit_asis_url)
Expand Down Expand Up @@ -81,6 +84,7 @@ export(knit2pandoc)
export(knit2pdf)
export(knit2wp)
export(knit_child)
export(knit_cnd_format)
export(knit_code)
export(knit_engines)
export(knit_exit)
Expand Down
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@

This engine also works for other types of documents (e.g., `Rnw`) but it will not allow for nested code chunks within the `verbatim` engine.

* New `knit_cnd_format()` generic. It is called by the condition methods for `sew()`. Methods should return a full condition message, including the prefix (e.g. `Error:` or `Warning:`, including a call if the condition includes one).

## BUG FIXES

- The chunk option `child` also respects the package option `root.dir` now (thanks, @salim-b, https://community.rstudio.com/t/117563).
Expand Down
51 changes: 45 additions & 6 deletions R/output.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,23 +529,62 @@ msg_sanitize = function(message, type) {

#' @export
sew.warning = function(x, options, ...) {
call = if (is.null(x$call)) '' else {
call = deparse(x$call)[1]
if (call == 'eval(expr, envir, enclos)') '' else paste(' in', call)
if (is_eval_call(x$call)) {
x$call = NULL
}
msg_wrap(sprintf('Warning%s: %s', call, conditionMessage(x)), 'warning', options)
msg_wrap(knit_cnd_format(x), 'warning', options)
}
is_eval_call = function(x) {
is.call(x) && identical(x[1], quote(eval()))
}

#' @export
sew.message = function(x, options, ...) {
msg_wrap(paste(conditionMessage(x), collapse = ''), 'message', options)
msg_wrap(knit_cnd_format(x), 'message', options)
Comment on lines -541 to +543
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the paste() here was required so that this works

A message in message(..., appendLF = FALSE) will be merged with the next adjacent message:

f3 = function() {
 message('Hello ', appendLF = FALSE)
 message('World!')
}
f3()

The change here currently make the test fails on this. The knitr-examples checks is showing the difference between previous and new behavior for example (https://github.com/yihui/knitr-examples/blob/master/117-messages.Rmd)
https://github.com/yihui/knitr/runs/4396175038?check_suite_focus=true#step:11:1669

Was it necessary to remove it in knit_cnd_format.message ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer Christophe!

Since it's not "in-API" to return a character vector from conditionMessage(), I overlooked that part. Now fixed, with unit test and comments. However this means that knit_cnd_format() methods for messages need to know about that message merging behaviour and the message field potentially being a character vector. That's a bit problematic.

By the way, merge_class() manipulates message fields instead of the result of conditionMessage(). This could cause issues in the future because conditions might have empty "" or dummy strings as message fields, with the actual message generated with a conditionMessage() method. It's not a big deal though because lazy generation of error messages mostly occurs with errors not messages. However it's worth knowing that logic isn't 100% correct.

I guess ideally knit_cnd_format() would be called inside the merge_class() routine so that it generates messages properly. A new object holding the merged messages would need to be created instead of modifying the existing condition, which is not hygienic because it may break class expectations. And then we don't need the collapsing in knit_cnd_format.message() or any methods for subclasses of messages.

That said, we don't need to fix this now (or maybe ever) since message subclasses are not that important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, is there a way to run the knitr-examples check locally?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To run them locally, you need to clone the repository and then knit the files.
Usually, we automate the testing using CI and only run locally when something seems off.

Otherwise, to reproduce the checks without trying to recreate the full environnemnt (not that easy, that is why I rely on CI as I struggled bc on Windows mainly), the script to run in knitr-examples/ is ./knitall after having installed dev version.
The full process is make integration in knitr folder, which should work if you have knitr-example at the right place inside the knitr folder. (what happens in the GHA workflow).

To run one file, the script is ./k

Anyway, relying on the CI to run those tests is fine also.

}

#' @export
sew.error = function(x, options, ...) {
msg_wrap(as.character(x), 'error', options)
msg_wrap(knit_cnd_format(x), 'error', options)
}

#' Format a condition prior to sewing
#'
#' This generic is called by condition methods for
#' \code{\link{sew}()}. Methods should return a full condition
#' message, including the prefix (e.g. `Error:` or `Warning:`,
#' including a call if the condition includes one).
#'
#' @keywords internal
#' @export
knit_cnd_format = function(cnd) {
UseMethod('knit_cnd_format')
}

#' @export
knit_cnd_format.message = function(cnd) {
# Character vectors are created by `merge_class()` to support
# https://github.com/yihui/knitr-examples/blob/master/117-messages.Rmd
paste(conditionMessage(cnd), collapse = '')
}

#' @export
knit_cnd_format.warning = function(cnd) {
call = conditionCall(cnd)
if (is.null(call)) {
call = ''
} else {
call = paste(' in', deparse(call))[1]
}
sprintf('Warning%s: %s', call, conditionMessage(cnd))
}

#' @export
knit_cnd_format.error = function(cnd) {
as.character(cnd)
}


#' @export
sew.recordedplot = function(x, options, ...) {
# figure number sequence for multiple plots
Expand Down
15 changes: 15 additions & 0 deletions man/knit_cnd_format.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

82 changes: 82 additions & 0 deletions tests/testit/test-output.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,85 @@ assert('knit_meta_add() adds meta objects with the correct number of labels', {
knit_meta(clean = TRUE)
(m %==% c('', '', 'a', 'b'))
})

assert('sew() handles conditions',
identical(
sew(simpleError('msg', call = call('foo')), list()),
'Error in foo(): msg\n'
),
identical(
sew(simpleWarning('msg', call = call('foo')), list()),
'Warning in foo(): msg\n'
),
identical(
sew(simpleMessage('msg', call = call('foo')), list()),
'msg\n'
)
)

local({
assert('sew() strips `eval()` calls stored in warnings', {
cnd = tryCatch(
warning = identity,
eval(quote(warning("foo")))
)
identical(
sew(cnd, list()),
'Warning: foo\n'
)
})
})

local({
assert('knit_cnd_format() formats conditions', {
cnd = simpleMessage('msg', call = quote(foo()))
identical(
knit_cnd_format(cnd),
'msg'
)
}, {
cnd = simpleWarning('msg', call = quote(foo()))
identical(
knit_cnd_format(cnd),
'Warning in foo(): msg'
)
}, {
cnd = simpleWarning('msg', call = quote({ foo; bar; baz }))
identical(
knit_cnd_format(cnd),
'Warning in {: msg'
)
}, {
cnd = simpleMessage('msg', call = quote(foo()))
identical(
knit_cnd_format(cnd),
'msg'
)
})
})

local({
rlang::local_bindings(
.env = globalenv(),
knit_cnd_format.knitr_foobar = function(cnd) 'dispatched!'
)
assert('sew() dispatches on knit_cnd_format() with condition objects', {
cnd = structure(
list(message = 'foo'),
class = c('knitr_foobar', 'error', 'condition')
)
identical(
sew(cnd, list()),
'dispatched!\n'
)
})
})

# Character vectors are created by `merge_class()` to support
# https://github.com/yihui/knitr-examples/blob/master/117-messages.Rmd
assert('knit_cnd_format.message() supports character vectors',
identical(
knit_cnd_format(simpleMessage(c("foo", "bar"))),
"foobar"
)
)