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

Minor test/doc/code refactoring #158

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Minor test/doc/code refactoring #158

merged 1 commit into from
Jun 19, 2024

Conversation

hadley
Copy link
Member

@hadley hadley commented Jun 19, 2024

No description provided.

@hadley hadley merged commit ab6b58c into main Jun 19, 2024
13 checks passed
@hadley hadley deleted the tidying-up branch June 19, 2024 13:50
Comment on lines -160 to -161

default_output_handler <- new_output_handler()
Copy link
Collaborator

@cderv cderv Jun 20, 2024

Choose a reason for hiding this comment

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

First, knitr uses this internal function name
https://github.com/yihui/knitr/blob/aa34f1b85f6bb899b1e23a72ebd64054afb3c695/R/utils.R#L860-L861

So removing it breaking knitr. I did not review this PR so did not test it. Only tried dev evaluate today.

Moreover, this was added some years ago on purpose for performance reason it seems
78059bb
Commit message

for a large number of expressions, evaluating new_output_handler() again and again can take some time and also unnecessary, so just create a default copy beforehand and use it

How should we proceed regarding knitr ? Can we revert this so that knitr is not impacted ? Or should we make knitr do differently ?

But this will mean that older knitr won't work with newer evaluate. Which is already the case IMO as I needed already to do two commits in knitr to prepare for new evaluate (yihui/knitr@5510d3f and yihui/knitr@f4d670f)

Copy link
Member Author

Choose a reason for hiding this comment

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

If knitr uses it, I can bring it back (but compute it in a safer way)

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