From a2c9184a992ef4c999de325431c1cc2696ec89a6 Mon Sep 17 00:00:00 2001 From: Ivan K Date: Mon, 21 Oct 2024 13:37:28 +0300 Subject: [PATCH 01/10] Respect shouldPrint when auto-printing from knitr 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. --- NAMESPACE | 2 ++ R/print.data.table.R | 16 +++++++--------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 50b81e8e6a..60ede1d9c4 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -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) +# else manual delayed registration from the onLoad hook S3method(dim, data.table) S3method(dimnames, data.table) S3method("dimnames<-", data.table) diff --git a/R/print.data.table.R b/R/print.data.table.R index 0bebfce9a5..611880458d 100644 --- a/R/print.data.table.R +++ b/R/print.data.table.R @@ -30,13 +30,8 @@ print.data.table = function(x, topn=getOption("datatable.print.topn"), # Other options investigated (could revisit): Cstack_info(), .Last.value gets set first before autoprint, history(), sys.status(), # topenv(), inspecting next statement in caller, using clock() at C level to timeout suppression after some number of cycles SYS = sys.calls() - if (length(SYS) <= 2L || # "> DT" auto-print or "> print(DT)" explicit print (cannot distinguish from R 3.2.0 but that's ok) - ( length(SYS) >= 3L && is.symbol(thisSYS <- SYS[[length(SYS)-2L]][[1L]]) && - as.character(thisSYS) == 'source') || # suppress printing from source(echo = TRUE) calls, #2369 - ( length(SYS) > 3L && is.symbol(thisSYS <- SYS[[length(SYS)-3L]][[1L]]) && - as.character(thisSYS) %chin% mimicsAutoPrint ) ) { + if (length(SYS) <= 2L) { # "> DT" auto-print or "> print(DT)" explicit print (cannot distinguish from R 3.2.0 but that's ok) return(invisible(x)) - # is.symbol() temp fix for #1758. } } if (!is.numeric(nrows)) nrows = 100L @@ -158,9 +153,6 @@ format.data.table = function(x, ..., justify="none") { do.call(cbind, lapply(x, format_col, ..., justify=justify)) } -mimicsAutoPrint = c("knit_print.default") -# add maybe repr_text.default. See https://github.com/Rdatatable/data.table/issues/933#issuecomment-220237965 - shouldPrint = function(x) { ret = (identical(.global$print, "") || # to save address() calls and adding lots of address strings to R's global cache address(x)!=.global$print) @@ -288,3 +280,9 @@ trunc_cols_message = function(not_printed, abbs, class, col.names){ domain=NA ) } + +# Maybe add a method for repr::repr_text. See https://github.com/Rdatatable/data.table/issues/933#issuecomment-220237965 +knit_print.data.table <- function(x, ...) { + if (!shouldPrint(x)) return(invisible(x)) + NextMethod() +} From 5a52ad9ac852dce06e0f40ae81317143969ebf1a Mon Sep 17 00:00:00 2001 From: Ivan K Date: Mon, 21 Oct 2024 17:50:20 +0300 Subject: [PATCH 02/10] Delay S3method(knit_print, data.table) for R < 3.6 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. --- R/onLoad.R | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/R/onLoad.R b/R/onLoad.R index ef96849e85..cbf272f8f3 100644 --- a/R/onLoad.R +++ b/R/onLoad.R @@ -66,6 +66,15 @@ lockBinding("rbind.data.frame",baseenv()) } } + if (session_r_version < "3.6.0") { + # no delayed registration support for NAMESPACE; perform it manually + if (isNamespaceLoaded("knitr")) { + registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) + } + setHook(packageEvent("knitr", "onLoad"), function(...) { + registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) + }) + } # Set options for the speed boost in v1.8.0 by avoiding 'default' arg of getOption(,default=) # In fread and fwrite we have moved back to using getOption's default argument since it is unlikely fread and fread will be called in a loop many times, plus they From f9e65e230748030f0818b48e14f905aedb10e2e3 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 3 Dec 2024 00:05:27 -0800 Subject: [PATCH 03/10] ws-only style --- R/onLoad.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/onLoad.R b/R/onLoad.R index cbf272f8f3..d918db152a 100644 --- a/R/onLoad.R +++ b/R/onLoad.R @@ -69,10 +69,10 @@ if (session_r_version < "3.6.0") { # no delayed registration support for NAMESPACE; perform it manually if (isNamespaceLoaded("knitr")) { - registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) + registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) } setHook(packageEvent("knitr", "onLoad"), function(...) { - registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) + registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) }) } From 34c7ea0fe8c244c600dc2d4339b6b4c2d0235e72 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 3 Dec 2024 00:07:32 -0800 Subject: [PATCH 04/10] put setHook() in a branch --- R/onLoad.R | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/R/onLoad.R b/R/onLoad.R index d918db152a..026327cd95 100644 --- a/R/onLoad.R +++ b/R/onLoad.R @@ -70,10 +70,11 @@ # no delayed registration support for NAMESPACE; perform it manually if (isNamespaceLoaded("knitr")) { registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) + } else { + setHook(packageEvent("knitr", "onLoad"), function(...) { + registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) + }) } - setHook(packageEvent("knitr", "onLoad"), function(...) { - registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) - }) } # Set options for the speed boost in v1.8.0 by avoiding 'default' arg of getOption(,default=) From bd7a35f252ef26dbcab6d84f548da9d5cad5c507 Mon Sep 17 00:00:00 2001 From: Ivan K Date: Tue, 3 Dec 2024 21:40:18 +0300 Subject: [PATCH 05/10] Position comment on the same line --- NAMESPACE | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 60ede1d9c4..6e6343c1dd 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -104,8 +104,7 @@ 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) -# else manual delayed registration from the onLoad hook +if (getRversion() >= "3.6.0") S3method(knitr::knit_print, data.table) # else manual delayed registration from the onLoad hook S3method(dim, data.table) S3method(dimnames, data.table) S3method("dimnames<-", data.table) From 2557fa2c5e273e753f91b156cbabef8590f4527b Mon Sep 17 00:00:00 2001 From: Ivan K Date: Tue, 3 Dec 2024 22:05:55 +0300 Subject: [PATCH 06/10] Restore the still-required #2369 condition --- R/print.data.table.R | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/R/print.data.table.R b/R/print.data.table.R index 611880458d..fe9da00f68 100644 --- a/R/print.data.table.R +++ b/R/print.data.table.R @@ -30,7 +30,9 @@ print.data.table = function(x, topn=getOption("datatable.print.topn"), # Other options investigated (could revisit): Cstack_info(), .Last.value gets set first before autoprint, history(), sys.status(), # topenv(), inspecting next statement in caller, using clock() at C level to timeout suppression after some number of cycles SYS = sys.calls() - if (length(SYS) <= 2L) { # "> DT" auto-print or "> print(DT)" explicit print (cannot distinguish from R 3.2.0 but that's ok) + if (length(SYS) <= 2L || # "> DT" auto-print or "> print(DT)" explicit print (cannot distinguish from R 3.2.0 but that's ok) + ( length(SYS) >= 3L && is.symbol(thisSYS <- SYS[[length(SYS)-2L]][[1L]]) && + as.character(thisSYS) == 'source') ) { # suppress printing from source(echo = TRUE) calls, #2369 return(invisible(x)) } } From f37571e411986705ff2a4449ee3caac0e7a1cb52 Mon Sep 17 00:00:00 2001 From: Ivan K Date: Thu, 5 Dec 2024 10:09:29 +0300 Subject: [PATCH 07/10] Regression test for #2369 Avoid breaking it again like in #6589 --- tests/autoprint.R | 12 ++++++++++++ tests/autoprint.Rout.save | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/tests/autoprint.R b/tests/autoprint.R index 1e4694668b..964af70895 100644 --- a/tests/autoprint.R +++ b/tests/autoprint.R @@ -43,3 +43,15 @@ DT[1,a:=10L][] # yes. ...[] == oops, forgot print(...) tryCatch(DT[,foo:=ColumnNameTypo], error=function(e) e$message) # error: not found. DT # yes DT # yes + +# Regression test for auto-printing suppression in source(), #2369 +local({ + f = tempfile(fileext = ".R") + on.exit(unlink(f)) + writeLines(c( + "library(data.table)", + "DT = data.table(a = 1)", + "DT[,a:=1] # not auto-printed" + ), f) + source(f, local = TRUE, echo = TRUE) +}) diff --git a/tests/autoprint.Rout.save b/tests/autoprint.Rout.save index 41aaa89655..6b0b641b0f 100644 --- a/tests/autoprint.Rout.save +++ b/tests/autoprint.Rout.save @@ -136,6 +136,24 @@ NULL 1: 10 2: 10 > +> # Regression test for auto-printing suppression in source(), #2369 +> local({ ++ f = tempfile(fileext = ".R") ++ on.exit(unlink(f)) ++ writeLines(c( ++ "library(data.table)", ++ "DT = data.table(a = 1)", ++ "DT[,a:=1] # not auto-printed" ++ ), f) ++ source(f, local = TRUE, echo = TRUE) ++ }) + +> library(data.table) + +> DT = data.table(a = 1) + +> DT[, `:=`(a, 1)] +> > proc.time() user system elapsed 0.223 0.016 0.231 From 652ee1b3a09a1aeb4191ea5955d57f9ed9452ba5 Mon Sep 17 00:00:00 2001 From: Ivan K Date: Thu, 5 Dec 2024 10:22:48 +0300 Subject: [PATCH 08/10] NEWS entry --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index 23213934fc..b939d059c4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -113,6 +113,8 @@ rowwiseDT( 13. `rbindlist(l, use.names=TRUE)` can now handle different encodings for the column names in different entries of `l`, [#5452](https://github.com/Rdatatable/data.table/issues/5452). Thanks to @MEO265 for the report, and Benjamin Schwendinger for the fix. +14. The auto-printing suppression in `knitr` documents is now done by implementing a method for `knit_print` instead of looking up the call stack, [#6589](https://github.com/Rdatatable/data.table/pull/6589). Thanks to @jangorecki for the report [#6509](https://github.com/Rdatatable/data.table/issues/6509) and @aitap for the fix. + ## NOTES 1. Tests run again when some Suggests packages are missing, [#6411](https://github.com/Rdatatable/data.table/issues/6411). Thanks @aadler for the note and @MichaelChirico for the fix. From eb54f9fc08ecf9d68255af1fc1f46e93d594a127 Mon Sep 17 00:00:00 2001 From: aitap Date: Thu, 5 Dec 2024 07:23:28 +0000 Subject: [PATCH 09/10] Comment the .onLoad condition Co-authored-by: Michael Chirico --- R/onLoad.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/onLoad.R b/R/onLoad.R index 026327cd95..972fad821c 100644 --- a/R/onLoad.R +++ b/R/onLoad.R @@ -66,7 +66,7 @@ lockBinding("rbind.data.frame",baseenv()) } } - if (session_r_version < "3.6.0") { + if (session_r_version < "3.6.0") { # corresponds to S3method() directive in NAMESPACE # no delayed registration support for NAMESPACE; perform it manually if (isNamespaceLoaded("knitr")) { registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) From 21a40e9ae5e1167c96aecf9e07c751ad5200d67b Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 4 Dec 2024 23:25:04 -0800 Subject: [PATCH 10/10] restore unconditional setHook() --- R/onLoad.R | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/R/onLoad.R b/R/onLoad.R index 972fad821c..662f3132ed 100644 --- a/R/onLoad.R +++ b/R/onLoad.R @@ -70,11 +70,10 @@ # no delayed registration support for NAMESPACE; perform it manually if (isNamespaceLoaded("knitr")) { registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) - } else { - setHook(packageEvent("knitr", "onLoad"), function(...) { - registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) - }) } + setHook(packageEvent("knitr", "onLoad"), function(...) { + registerS3method("knit_print", "data.table", knit_print.data.table, envir = asNamespace("knitr")) + }) } # Set options for the speed boost in v1.8.0 by avoiding 'default' arg of getOption(,default=)