Reinstate handlers during Rprofile sourcing#1201
Conversation
lionel-
left a comment
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?
There was a problem hiding this comment.
I honestly don't know, except that it doesn't return TRUE. I couldn't find whatever was causing it so ended up being pragmatic about it instead
There was a problem hiding this comment.
If you get a chance can you please try identical(as.list(zap_srcref(h)))? This should get rid of things like srcrefs / bytecode.
Maybe the environment is different too, but I can't see why that would be the case from the top of my head.
Not that big of a deal but all.equal() is certainly an eyebrow raiser ;)
| # 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.
There was a problem hiding this comment.
I guess I'm still a bit uncertain about the meaning of the various prefixes. Will clean up
| # `.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().
There was a problem hiding this comment.
The function now makes sure to be idempotent so reapplying it shouldn't change a thing. This is also why I think we need to reuse the function rather than split it because we would otherwise be dependent on the order of calling. I'd be fine with renaming initialize_errors() but don't see any upside to splitting it into two more or less identical functions
lionel-
left a comment
lionel-
left a comment
There was a problem hiding this comment.
One of the CI failure is transient and the other should be fixed on main.
If you need help with the integration test please feel free to come chat with me.
| existing_ps_handler <- vapply( | ||
| handlers, | ||
| function(h) { | ||
| isTRUE(all.equal(h, .ps.errors.globalErrorHandler)) || |
There was a problem hiding this comment.
If you get a chance can you please try identical(as.list(zap_srcref(h)))? This should get rid of things like srcrefs / bytecode.
Maybe the environment is different too, but I can't see why that would be the case from the top of my head.
Not that big of a deal but all.equal() is certainly an eyebrow raiser ;)
2 participants
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