-
-
Notifications
You must be signed in to change notification settings - Fork 219
Remove deprecated loadRcppModules (closes #1415) #1416
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
This change is coming up with a small repercussion: old(er) package rncl has an unused (!!) > db <- tools::CRAN_package_db()
> rdrncl <- data.table(Package = tools::package_dependencies("rncl", reverse=TRUE, recursive=TRUE, db=db)[[1]])
> rdrcpp <- data.table(Package = tools::package_dependencies("Rcpp", reverse=TRUE, recursive=TRUE, db=db)[[1]])
> rdrcpp[rdrncl, on="Package"][, Package]
[1] "CommEcol" "phylobase" "rotl" "bivariatemaps" "adephylo"
[6] "adiv" "DAISIEprep" "ddtlcm" "nodeSub" "PCPS"
[11] "phyext2" "phylocanvas" "phylosem" "phylosignal" "phyloTop"
[16] "taxize" "adespatial" "EcoCleanR" "taxotools" "OCNet"
[21] "eDITH" "rivnet"
> |
|
Oh, but it looks otherwise fine. Can you review please, now that I flipped it to normal non-draft? Rev.dep check will be done later today. |
c764ddd to
d74c691
Compare
Enchufa2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the comment above, I see a couple of places:
inst/skeleton/zzz.R:# loadRcppModules()
inst/tinytest/testRcppModule/R/zzz.R:# loadRcppModules()
These are chunks that have been long commented out, but maybe it's a good time to clean that up too.
| exposed in the package namespace. The special value `TRUE` means that all | ||
| objects are exposed. | ||
|
|
||
| ### Deprecated legacy method using loadRcppModules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole section should be removed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and done in the new commits now pushed.
|
Funny. I noticed commented-out chunks at other repos (when chasing the issue in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Enchufa2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Thanks for catching and highlighting those two issues I overlooked! |
|
Rev.dep check finished, report file committed tl;dr:
So net-net a single 'change to worse' which we can address (with cooperation of its maintainer, that is) |
This PR removes the R file defining
loadRcppModules()along with all documentation references to this long-deprecated function. Shrinking the package surface for once!Draft for the two or so days the reverse dependencies will noodle over this.Flipped the bit to normal as the reverse depends check is progressing nicely.Checklist
R CMD checkstill passes all tests