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

sew() method for warnings and messages #1551

Closed
lionel- opened this issue Feb 6, 2023 · 10 comments · Fixed by #1557
Closed

sew() method for warnings and messages #1551

lionel- opened this issue Feb 6, 2023 · 10 comments · Fixed by #1557
Labels

Comments

@lionel-
Copy link
Member

lionel- commented Feb 6, 2023

rlang/R/cnd-message.R

Lines 309 to 326 in 90ef6d1

on_load({
s3_register("knitr::sew", "rlang_error", function(x, options, ...) {
# Simulate interactive session to prevent full backtrace from
# being included in error message
local_interactive()
# Save the unhandled error for `rlang::last_error()`.
poke_last_error(x)
# Include backtrace footer option in the condition. Processed by
# `cnd_message()`.
x <- cnd_set_backtrace_on_error(x, peek_backtrace_on_error_report())
# The `sew.error()` method calls `as.character()`, which dispatches
# to `cnd_message()`
NextMethod()
})
})

@hadley
Copy link
Member

hadley commented Feb 6, 2023

I think warnings are most important as you want to get a traceback for them, since otherwise they're very difficult to pin down when you're not in an interactive context.

@lionel-
Copy link
Member Author

lionel- commented Feb 9, 2023

The tricky part is that we need to save the warnings that occurred in the current document and reset the list when a new document starts. Do you know if there's an easy way to achieve that @cderv? Is there a hook that runs when knitr starts knitting, that we could use to reset our warning list?

@lionel- lionel- added the cnd label Feb 9, 2023
@cderv
Copy link

cderv commented Feb 9, 2023

If I understand correctly this is related to this issue yihui/knitr#2219 where suggestion would be to make those method lives in knitr directly ? In that scenario, it would be probably easier to reset after knitting a document, right ?

Currently in knitr, we do have a way to store error, message and warnings with internal knit_log object. For context, it was introduced in 0.6

During knitting, all messages, warnings and errors are stored (it happens in msg_wrap() which is called by sew.error, sew.warning and sew.message. It is currently use to report a detailed logging report (which include also code chunk content).

Anyway, we have currently a way to store message, error and warning and the list is reset at the start of each knitting.

Would that be useful ? Probably more for an internal implementation than in rlang.

Is there a hook that runs when knitr starts knitting, that we could use to reset our warning list?

We don't have those kind of hooks, but probably something we could easily add.

So on both account, we can definitely adapt knitr so that this is feasible.

@lionel-
Copy link
Member Author

lionel- commented Feb 10, 2023

This is complicated and I was initially a bit confused regarding Hadley's needs. Here is a summary of the three main integration points between rlang and knitr:

It turns out that Hadley only needs rlang_backtrace_on_warning_report, not last_warnings(), so there's no urgency for the init/final hooks in knitr. But I think these would be good to have at some point in the future. last_warnings() support in Rmd may be useful for teaching the rlang features in a book or a vignette.

Given that these features are closely related to rlang (global options, last_error(), last_warnings()), I think the implementations should remain in rlang instead of being moved to knitr.

@cderv
Copy link

cderv commented Feb 10, 2023

Thanks Lionel.

so there's no urgency for the init/final hooks in knitr. But I think these would be good to have at some point in the future. last_warnings() support in Rmd may be useful for teaching the rlang features in a book or a vignette.

So if I understand correctly, the only thing that would help at the knitr level would be adding some init and final hooks for knitr::knit() so that other package like rlang can do some setup and cleanup. This would be a generic feature.

We could similarly implement support for last_warnings(), but then we'd need the hooks I mentioned in the previous comment.

Previous comment being

The tricky part is that we need to save the warnings that occurred in the current document and reset the list when a new document starts.

If you need access to the warnings, would the knit_log() object be useful if you could access it from a rlang function ?
Or do you need anyway another list with different stored warning ?

@lionel-
Copy link
Member Author

lionel- commented Feb 10, 2023

It looks like we could use knit_log() to emulate the main feature of last_warnings() (displaying warnings and traces), but it wouldn't be a full implementation because knit_log() returns strings instead of condition objects.

I think we could fully implement last_warnings() and simplify the implementation of last_error() if knitr provided knit_log$get_cnds() to get the raw conditions that were captured by knitr/evaluate.

@lionel-
Copy link
Member Author

lionel- commented Feb 10, 2023

@cderv I just figured out I can use the same trick to detect new knitr sessions than we already do in last_warnings() to detect new commands. We can just compare the pointer of the outermost knitr frame, if it doesn't correspond, it's a new session and we start a new list of warnings.

@cderv
Copy link

cderv commented Feb 10, 2023

Oh clever. Learning a lot by seeing how rlang works and how you solve this. Thanks for sharing thoughts process.

knit_log() returns strings instead of condition objects.

Oh now I see what you need. Indeed we only use the message internal that we format. BTW knit_log is a special special "object" in knitr that works using knit_log$set() and knit_log$get() then knit_log$restore() for resetting to default.

if knitr provided knit_log$get_cnds() to get the raw conditions

I think it would be possible to provide something like this. Several ways to make that happen. But you don't need anymore it now right ? Using the pointer solves your issue right ?

I still think the init / final hooks are a good thing to have though. I'll keep that in mind if we have a new usage that comes

@lionel-
Copy link
Member Author

lionel- commented Feb 10, 2023

But you don't need anymore it now right ? Using the pointer solves your issue right ?

yup it's fixed in #1557.

I still think the init / final hooks are a good thing to have though. I'll keep that in mind if we have a new usage that comes

Sounds good!

@cderv
Copy link

cderv commented Feb 10, 2023

Awesome thanks. All good. That is really cool feature that are coming in next rlang !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants