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

Eliminate enclose argument #137

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Eliminate enclose argument #137

merged 4 commits into from
Jun 18, 2024

Conversation

hadley
Copy link
Member

@hadley hadley commented Jun 16, 2024

This eliminates one argument we need to thread through multiple function calls by creating a new environment with appropriate parent. (I doubt anyone actually uses this argument but it should be a safe transformation)

This eliminates one argument we need to thread through multiple function calls by creating a new environment with appropriate parent. (I doubt anyone actually uses this argument but it should be a safe transformation)
@hadley hadley requested a review from cderv June 16, 2024 17:00
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

Seems ok.

We need to update knitr sew.warning function so that the enclos is not expected anymore.

Currently it is

#' @export
sew.warning = function(x, options, ...) {
  call = if (is.null(x$call)) '' else {
    call = deparse(x$call)[1]
    if (call == 'eval(expr, envir, enclos)') '' else paste(' in', call)
  }
  msg_wrap(sprintf('Warning%s: %s', call, conditionMessage(x)), 'warning', options)
}

Source: https://github.com/yihui/knitr/blob/e1edd34f4149d996efce5cf539e781eda6e3b86a/R/output.R#L557

For context on this, I am running your PR against knitr-examples repo and knitr test suits to see which impact it could have. It helps me find the possible problems like this one

Diff we get from knitr-examples
--- a/007-text-output.md
+++ b/007-text-output.md
@@ -477,7 +477,7 @@ warning("no no no")
 ```
 
 ```
-## Warning: no no no
+## Warning in eval(expr, envir): no no no
 ```
 
 Invalid indices will select nothing:

@hadley
Copy link
Member Author

hadley commented Jun 18, 2024

Hmmmmm, I think we should be able to fix that in evaluate() so knitr continues to work, and you can eventually remove the existing code. I'll file a new issue momentarily.

@hadley hadley merged commit ba8f5bc into main Jun 18, 2024
13 checks passed
@hadley hadley deleted the eliminate-enclos branch June 18, 2024 16:18
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.

2 participants