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

Ensure global defers run on exit #173

Merged
merged 8 commits into from
Nov 4, 2021
Merged

Ensure global defers run on exit #173

merged 8 commits into from
Nov 4, 2021

Conversation

hadley
Copy link
Member

@hadley hadley commented Sep 1, 2021

Opening this primarily for discussion — this change means that when withr::defer() is called interactively (i.e. from the global environment), it will be run when you end the session.

I'm not 100% sure it's a good idea (or if it's been discussed and rejected before) but this is how I expected it to wrk. I started looking into because I noticed that my tests weren't always running cleanup code, and that lead to a simple experiment confirming that withr::defer(print("Hi")) is not executed when you quit R.

cc @jennybc, @jimhester, @lionel-

@jennybc
Copy link
Member

jennybc commented Sep 1, 2021

We have discussed this before, on the original PR, starting around here.

@jennybc
Copy link
Member

jennybc commented Sep 1, 2021

In terms of the workflow aspect, I have something in my .Rprofile that makes dr a shortcut for withr::deferred_run(), which I use heavily when I'm developing, e.g. tests, that use defer().

@jennybc
Copy link
Member

jennybc commented Sep 1, 2021

See also #136 no, actually not related

@hadley
Copy link
Member Author

hadley commented Sep 1, 2021

I now feel more strongly that the non-finalizer approach is suboptimal because it relies on me to remember that I have to run deferred_run(), which is very hard.

@jennybc
Copy link
Member

jennybc commented Sep 1, 2021

I don't have a strong opinion about this. I guess I prefer the status quo and I am certainly very used to it, but I can also make peace with the proposed change.

@lionel-
Copy link
Member

lionel- commented Sep 2, 2021

The session finalizer comes in addition to the existing features, so I guess it makes sense. I wouldn't rely on it being run for important things though.

In general I don't have any strong feeling about global defer() except that we need to fix #136 so that the message isn't displayed in non-top-level global evaluations which are actually cleaned up automatically (I'm still happy to send a PR).

@hadley hadley marked this pull request as ready for review September 7, 2021 16:39
@hadley
Copy link
Member Author

hadley commented Sep 7, 2021

@jimhester if you want a test, I think I'd have to add a dependency on callr. I can't see any other way except to start another R process.

NEWS.md Outdated
@@ -1,5 +1,9 @@
# withr (development version)

* Handlers registered with the global environment (as happens when `local_()`
is run at the top-level, outside a function) are now automatically run
when the R session ends.
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the PR number here?

@jimhester
Copy link
Member

jimhester commented Sep 7, 2021

I think it is ok to skip the test here.

OTOH callr is a testthat dependency anyway, so adding it here for the tests is basically free.

@hadley
Copy link
Member Author

hadley commented Sep 7, 2021

Will need an update after #176

@hadley hadley merged commit f25fb5f into main Nov 4, 2021
@hadley hadley deleted the global-finalize branch November 4, 2021 13:50
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.

4 participants