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

Remove redundant S3 registration code #136

Merged
merged 2 commits into from
Aug 26, 2019

Conversation

wch
Copy link
Collaborator

@wch wch commented Aug 26, 2019

It turns out that #108 was redundant - essentially the same code already existed at

htmltools/R/tags.R

Lines 22 to 57 in b5b1a6e

# Reusable function for registering a set of methods with S3 manually. The
# methods argument is a list of character vectors, each of which has the form
# c(package, genname, class).
registerMethods <- function(methods) {
lapply(methods, function(method) {
pkg <- method[[1]]
generic <- method[[2]]
class <- method[[3]]
func <- get(paste(generic, class, sep="."))
if (pkg %in% loadedNamespaces()) {
registerS3method(generic, class, func, envir = asNamespace(pkg))
}
setHook(
packageEvent(pkg, "onLoad"),
function(...) {
registerS3method(generic, class, func, envir = asNamespace(pkg))
}
)
})
}
.onLoad <- function(...) {
# htmltools provides methods for knitr::knit_print, but knitr isn't a Depends or
# Imports of htmltools, only an Enhances. Therefore, the NAMESPACE file has to
# declare it as an export, not an S3method. That means that R will only know to
# use our methods if htmltools is actually attached, i.e., you have to use
# library(htmltools) in a knitr document or else you'll get escaped HTML in your
# document. This code snippet manually registers our methods with S3 once both
# htmltools and knitr are loaded.
registerMethods(list(
# c(package, genname, class)
c("knitr", "knit_print", "html"),
c("knitr", "knit_print", "shiny.tag"),
c("knitr", "knit_print", "shiny.tag.list")
))
}
. This removes the extra copy.

@wch wch requested a review from jcheng5 August 26, 2019 15:52
@wch wch force-pushed the wch-remove-redundant-s3 branch 2 times, most recently from 4c0c7d0 to 2dde5c7 Compare August 26, 2019 16:43
@wch wch added this to the v0.4.0 milestone Aug 26, 2019
@jcheng5 jcheng5 merged commit 01f3a69 into rstudio:master Aug 26, 2019
@wch wch deleted the wch-remove-redundant-s3 branch August 27, 2019 01:36
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.

2 participants