Skip to content

Reinstate handlers during Rprofile sourcing#1201

Open
thomasp85 wants to merge 1 commit into
posit-dev:mainfrom
thomasp85:issue-1101-message-color-startup
Open

Reinstate handlers during Rprofile sourcing#1201
thomasp85 wants to merge 1 commit into
posit-dev:mainfrom
thomasp85:issue-1101-message-color-startup

Conversation

@thomasp85
Copy link
Copy Markdown
Collaborator

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 noisy

pushing duplicate `error` handler on top of the stack
pushing duplicate `warning` handler on top of the stack
pushing duplicate `message` handler on top of the stack
pushing duplicate `interrupt` handler on top of the stack

during 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

message("Hello")
globalCallingHandler(test = function(...) print("test"))

and check that "Hello" is printed in white, not red, and that a "test" handler has been correctly registered

Copy link
Copy Markdown
Contributor

@lionel- lionel- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ark rather than ps

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

message() output during R startup appears red in Positron console

2 participants