use .datatable.aware when data.table in Sugggests #3818
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3818 +/- ##
=======================================
Coverage 99.41% 99.41%
=======================================
Files 71 71
Lines 13265 13265
=======================================
Hits 13188 13188
Misses 77 77Continue to review full report at Codecov.
|
| @@ -82,6 +82,8 @@ test_that("generate dt", { expect_true(nrow(gen()) == 100) }) | |||
| test_that("aggregate dt", { expect_true(nrow(aggr(gen())) < 100) }) | |||
| ``` | |||
|
|
|||
| If `data.table` is in Suggests (but not Imports) then you need to declare `.datatable.aware=TRUE` in one of the R/* files to avoid "object not found" errors when testing via `testthat::test_package` or `testthat::test_check`. | |||
There was a problem hiding this comment.
could you confirm this statement is also valid when NAMESPACE is present? to not have it blank you can import anything from base R, or export dummy function there
There was a problem hiding this comment.
works with blank NAMESPACE, and when I do importFrom("stats", "sd")
| When using a package as a suggested dependency, you should not `import` it in the `NAMESPACE` file. Just mention it in the `DESCRIPTION` file. You also _must_ use the `data.table::` prefix when calling `data.table` functions because none of them are imported. | ||
| When using a package as a suggested dependency, you should not `import` it in the `NAMESPACE` file. Just mention it in the `DESCRIPTION` file. | ||
| When using `data.table` functions in package code (R/* files) you need to use the `data.table::` prefix because none of them are imported. | ||
| When using `data.table` in package tests (e.g. tests/testthat/test* files), you need to declare `.datatable.aware=TRUE` in one of the R/* files. |
There was a problem hiding this comment.
same here, good to know if presence of NAMESPACE file matters
There was a problem hiding this comment.
I don't think NAMESPACE matters. presence of .datatable.aware=TRUE means no error on testthat::test_package (as expected); absence means unexpected "object not found" error.
There was a problem hiding this comment.
I think all this can be answered with cedta code:
isTRUE(ns$.datatable.aware)
where ns is the namespace of the call, so .datatable.aware covers all bases.
Earlier we see:
"data.table" %chin% names(getNamespaceImports(ns))
# getNamespaceImports main code:
.getNamespaceInfo(ns, "imports")
AFAICT these names correspond to importFrom or import statements in NAMESPACE
There was a problem hiding this comment.
not sure I understand. can you please clarify? are you saying that you may be able to modify cedta to fix the issue, without having me add .datatable.aware=TRUE in my package? that would be ideal.
There was a problem hiding this comment.
no, I think without being in imports, .datatable.aware is probably the canonical way to do things. .datatable.aware=TRUE seems pretty minimal after all! Just a quick line of code to signal to data.table that you know we're here.
My comment was hoping to shed light on the discussion on whether NAMESPACE matters
Hi this PR was requested by @jangorecki after a short discussion on the linked issue (#2053). The change to the vignette adds some discussion about what to do when data.table is in Suggests (not imported) and used only in tests.