-
-
Notifications
You must be signed in to change notification settings - Fork 872
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
Add knit_cnd_format()
generic
#2078
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small feedback as I observe a change in the test.
Otherwise, this looks good. I let @yihui add his review which will be more relevant for knitr internals. Thanks Lionel !
msg_wrap(paste(conditionMessage(x), collapse = ''), 'message', options) | ||
msg_wrap(knit_cnd_format(x), 'message', options) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
a8e2842
to
2e37579
Compare
rlang is now in charge of fully printing error messages, including the error prefix. This makes it possible to support more features, such as custom display of condition calls and chained errors.
Ideally, the condition messages in knitted documents should match the messages in the REPL. This is currently achieved for errors via an
as.character.rlang_error()
method that is called bysew.error()
. However, methods for other conditions do not callas.character()
. Also that customisation point is not very explicit and is probably to be considered off-label usage from the point of view of knitr.To solve these issues, this PR proposes to add a new generic for the full formatting of condition messages, including the prefix. Example usage can be found in https://github.com/r-lib/rlang/pull/1323/files.
cc @hadley