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

Unregister S3 methods explicitly #1001

Merged
merged 4 commits into from Dec 24, 2015

Conversation

Projects
None yet
6 participants
@jimhester
Member

jimhester commented Dec 23, 2015

This is a workaround for a bug in base R. unloadNamespace does not
unregister any S3 methods, so the promises are invalid if you install a
new version of a package, try to unload and re-load the package.

See https://stat.ethz.ch/pipermail/r-devel/2015-December/072150.html for
more details and a reproducible example.

This fixes the common lazy-load errors people often run into when trying
to use devtools' install functions.

Fixes #419, #503, #942, #631

Unregister S3 methods explicitly
This is a workaround for a bug in base R. unloadNamespace does not
unregister any S3 methods, so the promises are invalid if you install a
new version of a package, try to unload and re-load the package.

See https://stat.ethz.ch/pipermail/r-devel/2015-December/072150.html for
more details and a reproducible example.

This fixes the common lazy-load errors people often run into when trying
to use devtools' install functions.

Fixes #419, #503, #942, #631
@jimhester

This comment has been minimized.

Member

jimhester commented Dec 23, 2015

While this is working for the test package and for devtools itself I have not tested it with any S4 packages to ensure it works there. I wanted to put it out there to get feedback on the implementation and see if there are any changes that need to be made.

This error has long been a frustrating one for me, as it seemed to happen randomly, so finally figuring out what was going on and fixing it was pretty satisfying!

@wch

This comment has been minimized.

Member

wch commented Dec 23, 2015

Nice! If this is indeed a reliable fix that's compatible with S4 stuff, it'll be great to have those errors go away.

@jimhester

This comment has been minimized.

Member

jimhester commented Dec 23, 2015

200b631 uses a different approach to fixing the issue by simply forcing all of the promises explicitly before trying to unload rather than removing them. I also converted all of the previous uses of as.list() to use eapply(ns, force) to make it a little more explicit what was going on.

The previous commit was running into problems when trying to unload or reinstall devtools that do not occur using this method.

I also tested this version on some S4 classes from Bioconductor and they seem to work without error.

@hadley

This comment has been minimized.

Member

hadley commented Dec 23, 2015

I like it. I think the only thing that's missing is a comment with the code explaining your reasoning above

@jimhester

This comment has been minimized.

Member

jimhester commented Dec 24, 2015

I actually had a comment written (6d760c5) just neglected to push it. I also added a note to the NEWS about this change d96101f.

Let me know if you want me to squash the commits or leave them as-is.

hadley added a commit that referenced this pull request Dec 24, 2015

Merge pull request #1001 from jimhester/unload_lazy
Unregister S3 methods explicitly.

Fixes #419, #503, #942, #631

@hadley hadley merged commit 8ad299f into r-lib:master Dec 24, 2015

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hadley

This comment has been minimized.

Member

hadley commented Dec 24, 2015

Thanks!

@jimhester

This comment has been minimized.

Member

jimhester commented Dec 28, 2015

For reference I opened a bug report about this (https://bugs.r-project.org/bugzilla3/show_bug.cgi?id=16644). BDR points out it is technically documented behavior, so there is unlikely to be a fix upstream.

Personally I think the current behavior of detach() and unloadNamespace() is hostile to users. You should be able to first load and unload a package and have your environment be identical to when you started. It doesn't even seem that difficult to add this support, so I wonder why it wasn't implemented (I am probably missing some subtlety).

Anyway this pull should fix the majority of the errors people experience when using devtools at least, thank you for merging it!

@hadley

This comment has been minimized.

Member

hadley commented Dec 28, 2015

@jimhester thanks for digging into this - it's been a hassle for ages!

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Dec 28, 2015

++ 💯

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Dec 28, 2015

Btw. this makes me want to create an unloader package. :)

@brodieG

This comment has been minimized.

brodieG commented Dec 30, 2015

@jimhester bloody awesome. Best X-mas present this year. So sick and tired of the install-test-install-corrupt-db-quit-restart-crap-didn't-save-workspace cycle.

@sanjmeh

This comment has been minimized.

sanjmeh commented Jun 19, 2017

I am still facing the same frustrating install-test-install-corrupt-db-quit-restart-crap-didn't-save-workspace cycle. Can one of the experts here document the solution or attach a link to Stack overflow where this is addressed to your satisfaction? Apologies if this is already resolved and documented somewhere else.

@timtrice timtrice referenced this pull request Jul 10, 2017

Closed

rrricanes #118

11 of 14 tasks complete

@statsccpr statsccpr referenced this pull request Sep 17, 2017

Closed

Problem with help manual? #25

@mwmclean mwmclean referenced this pull request Sep 24, 2017

Closed

GetPubMedByID #37

@ChrisMuir ChrisMuir referenced this pull request Oct 6, 2017

Closed

Submission: comtradr #141

11 of 14 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment