Solve the knitr auto-printing problem by registering a method for knit_print
#6589
Conversation
Implementing a method for the knitr::knit_print generic makes it possible to customise the behaviour without looking up the call stack. The current solution only works on R >= 3.6.0 because that's where delayed S3 registration has been introduced.
Use setHook() to ensure that registerS3method() will be called in the same session if 'knitr' is loaded later. Not needed on R >= 3.6.0 where S3method(knitr::knit_print) will do the right thing by itself.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6589 +/- ##
==========================================
- Coverage 98.60% 98.60% -0.01%
==========================================
Files 79 79
Lines 14516 14514 -2
==========================================
- Hits 14314 14312 -2
Misses 202 202 ☔ View full report in Codecov by Sentry. |
| @@ -104,6 +104,8 @@ if (getRversion() >= "4.0.0") { | |||
| # version of R (and that is checked in .onLoad with error if not). | |||
| export(.rbind.data.table) # only export in R<4.0.0 where it is still used; R-devel now detects it is missing doc, #5600 | |||
| } | |||
| if (getRversion() >= "3.6.0") S3method(knitr::knit_print, data.table) | |||
There was a problem hiding this comment.
what happens if knitr is not installed?
IIUC this is an Enhances dependency; weve done something similar for {lintr}
https://github.com/r-lib/lintr/blob/1db7cc70655a41c77fcc89dbed5e40d5dd8cf74a/R/zzz.R#L337
There was a problem hiding this comment.
data.table still loads with no warnings. R CMD check fails even with _R_CHECK_FORCE_SUGGESTS_=FALSE because it's the VignetteBuilder.
WRE says:
function
gen.clswill get registered as an S3 method for classclsand genericgenfrom packagepkgonly when the namespace of pkg is loaded
Here's the code:
There was a problem hiding this comment.
even with R_CHECK_FORCE_SUGGESTS=FALSE because it's the VignetteBuilder.
Right, we need the flags to skip vignettes too, e.g. --no-build-vignettes --no-vignettes
Anyway, my concern was about loading {data.table}, thanks for the WRE cite!
I still think we should put {knitr} into Enhances:, WDYT?
There was a problem hiding this comment.
On the one hand, WRE says
the
Enhancesfield lists packages “enhanced” <...> by providing methods for classes from these packages
...which is a good argument for Enhances, but then it says
A package should be listed in only one of these fields.
The
VignetteBuilder<...> may include the current package, or ones listed inDepends,SuggestsorImports.
If we switch to litedown as the vignette builder, knitr should definitely go into Enhances. Currently, it looks like knitr and markdown should be in both Suggests and VignetteBuilder, although the latter doesn't seem to be tested. (E.g. cmocean had VignetteBuilder: knitr for 5 years despite having %\VignetteEngine{knitr::rmarkdown} and not a single test flagged it.)
Although it's of course possible to read WRE differently, in which case knitr could move from Suggests to Enhances.
It turns out that there is a better solution for #6566 after all. It's documented in
vignette('knit_print')that packages can overrideknitr's (auto-)printing behaviour by registering a method for it. R ≥ 3.6 can perform "delayed registration" of S3 methods for a generic in a package without requiring it to be loaded, hence keepingknitrinSuggests:. In R ∈ [3.3, 3.6), the same thing can be done manually: I've reused a piece of code frombase::registerS3methodsto registerknit_print.data.tableif the package is already loaded and set a hook to register it later if needed.The method itself just checks
shouldPrint(x)and returnsinvisible(x)if not, otherwise delegates to the default method.This can close #6566 if we don't want to change the behaviour of
.Primitive("[")with regards to visibility.