Implement S7_on_unload()#698
Conversation
Perform the opposite role as `S7_on_load()`. Fixes #316.
|
@t-kalinowski this doesn't need a full review, but I've gotten stuck in the weeds of refactoring and I think it would be useful to get some high level feedback before I go too far. |
t-kalinowski
left a comment
t-kalinowski
left a comment
There was a problem hiding this comment.
It looks good to me so far, I like this approach.
|
I'll do another round of refactoring as part of #660, but the tests are much easier to understand now. |
There was a problem hiding this comment.
I reviewed this and think the PR is good to go. I pushed a few additions while looking through unload cleanup. The main change is that S7_on_unload() now only unregisters an external S7 method if the unloading package still owns the active method.
There are a few supporting changes for that: package-registered S7 methods now carry owner metadata, unload skips external generics when the upstream package is unavailable or too old for the declared minimum version, and hook cleanup still removes stale hooks after an external method has been deleted. I added regression tests for those cases and updated NEWS/docs to match.
Please take a close look at the additions. If you think some of this is more edge-case handling than we want in this PR, I’m fine with dropping it.
|
Yeah, I'm not sure I would have gone that far, but it makes sense, and no need to undo it 😄 |
2 participants
Perform the opposite role as
S7_on_load(). Fixes #316.NB: for a full fix, this requires pkgload 1.5.3 which now actually calls the unload hooks.