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

Fix deferred_run() in knitr #251

Merged
merged 4 commits into from
Jan 16, 2024
Merged

Fix deferred_run() in knitr #251

merged 4 commits into from
Jan 16, 2024

Conversation

lionel-
Copy link
Member

@lionel- lionel- commented Jan 15, 2024

Branched from #250.
Closes #235.

Throwing an error causes a testthat vignette to fail (see r-lib/testthat#1920). And now that we no longer use exit_frame() as the public interface for handling environments specially, it's easier to introduce special-casing for knitr. We now use the old approach of attaching a list of handlers to the knitr exit frame. This way our handlers are not mixed with knitr's own handlers and we can flush them separately.

Fixes the testthat failure in revdeps.

@lionel- lionel- requested a review from hadley January 15, 2024 13:49
@hadley
Copy link
Member

hadley commented Jan 15, 2024

Should we include this in a vignette or test it some other way?

@lionel- lionel- changed the base branch from bugfix/rlang-defer to main January 16, 2024 07:46
@lionel-
Copy link
Member Author

lionel- commented Jan 16, 2024

Should we include this in a vignette or test it some other way?

I added a snapshot test before and now I've also turned on evaluation for the deferred_run() example in the vignette.

@lionel- lionel- merged commit bf5f21b into main Jan 16, 2024
11 checks passed
@lionel- lionel- deleted the feature/deferred-run-knitr branch January 16, 2024 07:58
@@ -311,28 +311,21 @@ It's hard to develop with functions that work one way inside a function, but ano

Here's how `defer()` (and all functions based on it) works in an interactive session.

```{r eval = FALSE}
```{r}
library(withr)

defer(print("hi"))
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to add this comment back as this example is about running in the global environment https://withr.r-lib.org/dev/articles/changing-and-restoring-state.html#deferring-events-on-the-global-environment, and we should show the user what they get. I think you could add that in a separate chunk that said "because this code is running in a vignette it doesn't look exactly the same as what you'll see. When you run it interactively, you'll also see this message: ..."

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, done in 546d0c2

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.

Should deferred_run() work within knitr?
2 participants