Reinstate handlers during Rprofile sourcing#1201
Conversation
lionel-
left a comment
There was a problem hiding this comment.
It would be nice to add some integration tests here (e.g. look at https://github.com/posit-dev/ark/blob/main/crates/ark/tests/kernel-r-profile.rs)
| # Unregister all handlers and hold onto them | ||
| handlers <- globalCallingHandlers(NULL) | ||
|
|
||
| existing_ps_handler <- vapply( |
There was a problem hiding this comment.
registered_ark_handlers
"registered" is clearer to me, and plural makes it clearer this is a vector
| existing_ps_handler <- vapply( | ||
| handlers, | ||
| function(h) { | ||
| isTRUE(all.equal(h, .ps.errors.globalErrorHandler)) || |
There was a problem hiding this comment.
What is preventing usage of identical() here?
| # message/error/warning rendering to match interactive behavior during | ||
| # `.Rprofile` sourcing without putting calling-handlers on the stack that | ||
| # would block user `globalCallingHandlers()` calls. | ||
| .ps.errors.source_with_handlers <- function(file) { |
There was a problem hiding this comment.
This is not exported so shouldn't use .ps. prefix.
| # `.Rprofile` sourcing without putting calling-handlers on the stack that | ||
| # would block user `globalCallingHandlers()` calls. | ||
| .ps.errors.source_with_handlers <- function(file) { | ||
| initialize_errors() |
There was a problem hiding this comment.
We'll need a comment in initialize_errors() to mention that particular calling context.
Are we certain that no ill side effect comes from reregistering the global handlers in that top-level context? Does R restore the handler stack to the previous state? (It might very well do that).
To make things clearer I think I'd prefer not using initialize_errors() which in my mind should only be called once per file. Probably best to split into initialize_global_handlers().
Fix #1101
This PR ensures that our error, warning, and message handlers are all in place during startup-sourcing of .Rprofile etc.
It does this by calling
initialize_errors()prior to sourcing which re-registers our handlers. It was necessary to modify the logic somewhat to avoid very noisyduring startup, so we now filter the existing handlers before applying our own (again). Trying to surpress this by wrapping in
suppressWarnings()etc results in system crash.To test this PR add this to you .Rprofile
and check that "Hello" is printed in white, not red, and that a "test" handler has been correctly registered