r-lib / rlang

Low-level API for programming with R
https://rlang.r-lib.org
Other
500 stars 137 forks source link

`sew()` method for warnings and messages #1551

Closed lionel- closed 1 year ago

lionel- commented 1 year ago

https://github.com/r-lib/rlang/blob/90ef6d1af8256ee0a13f95fad48d8c8fd489e7db/R/cnd-message.R#L309-L326

hadley commented 1 year ago

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- commented 1 year ago

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?

cderv commented 1 year ago

If I understand correctly this is related to this issue https://github.com/yihui/knitr/issues/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

  • all messages, errors and warnings during evaluation are recorded in an internal object knit_log (use knitr:::knit_log$get() to get all messages) and they are printed if the package option verbose is TRUE (i.e. opts_knit$get('verbose')) (yihui/knitr#224)

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- commented 1 year ago

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 commented 1 year ago

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- commented 1 year ago

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- commented 1 year ago

@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 commented 1 year ago

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- commented 1 year ago

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 commented 1 year ago

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