From d31641c5518ff97d487235847b221a8f36e49ac9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Ver=C3=ADssimo?=
 <211358+averissimo@users.noreply.github.com>
Date: Fri, 15 Nov 2024 12:30:25 +0100
Subject: [PATCH 1/9] feat: add warning on reserved datanames

---
 R/utils.R                         | 55 +++++++++++++++++++++++++++++--
 R/zzz.R                           |  1 +
 man/check_modules_datanames.Rd    |  3 ++
 man/pluralize.Rd                  | 23 +++++++++++++
 man/teal_data_module.Rd           |  4 ++-
 tests/testthat/test-module_teal.R | 28 ++++++++++++++++
 6 files changed, 110 insertions(+), 4 deletions(-)
 create mode 100644 man/pluralize.Rd

diff --git a/R/utils.R b/R/utils.R
index 6c3f8de083..b2890c262c 100644
--- a/R/utils.R
+++ b/R/utils.R
@@ -134,14 +134,40 @@ check_modules_datanames <- function(modules, datanames) {
 }
 
 #' @rdname check_modules_datanames
-check_modules_datanames_html <- function(modules,
-                                         datanames) {
+check_reserved_datanames <- function(datanames) {
+  reserved_datanames <- datanames[datanames %in% getOption("teal.reserved_datanames")]
+  if (length(reserved_datanames) == 0L) {
+    return(NULL)
+  }
+
+  tags$span(
+    to_html_code_list(reserved_datanames),
+    sprintf(
+      "%s reserved for internal use. Please avoid using %s as %s.",
+      pluralize(reserved_datanames, "is", "are"),
+      pluralize(reserved_datanames, "it", "them"),
+      pluralize(reserved_datanames, "a dataset name", "dataset names")
+    )
+  )
+}
+
+#' @rdname check_modules_datanames
+check_modules_datanames_html <- function(modules, datanames) {
   check_datanames <- check_modules_datanames_recursive(modules, datanames)
   show_module_info <- inherits(modules, "teal_modules") # used in two contexts - module and app
+
+  reserved_datanames <- check_reserved_datanames(datanames)
+
   if (!length(check_datanames)) {
-    return(TRUE)
+    out <- if (is.null(reserved_datanames)) {
+      TRUE
+    } else {
+      reserved_datanames
+    }
+    return(out)
   }
   shiny::tagList(
+    reserved_datanames,
     lapply(
       check_datanames,
       function(mod) {
@@ -445,3 +471,26 @@ build_datanames_error_message <- function(label = NULL,
     }
   )
 }
+
+#' Pluralize a word depending on the size of the input
+#'
+#' @param x (`object`) to check length for plural.
+#' @param singular (`character`) singular form of the word.
+#' @param plural (optional `character`) plural form of the word. If not given an "s"
+#' is added to the singular form.
+#'
+#' @return A `character` that correctly represents the size of the `x` argument.
+#' @keywords internal
+pluralize <- function(x, singular, plural = NULL) {
+  checkmate::assert_string(singular)
+  checkmate::assert_string(plural, null.ok = TRUE)
+  if (length(x) == 1L) { # Zero length object should use plural form.
+    singular
+  } else {
+    if (is.null(plural)) {
+      sprintf("%ss", singular)
+    } else {
+      plural
+    }
+  }
+}
diff --git a/R/zzz.R b/R/zzz.R
index 62c8029561..a2cba1b4d2 100644
--- a/R/zzz.R
+++ b/R/zzz.R
@@ -4,6 +4,7 @@
   teal_default_options <- list(
     teal.show_js_log = FALSE,
     teal.lockfile.mode = "auto",
+    teal.reserved_datanames = c("all", ".raw_data"),
     shiny.sanitize.errors = FALSE
   )
 
diff --git a/man/check_modules_datanames.Rd b/man/check_modules_datanames.Rd
index b01270eae2..f1df42ef2a 100644
--- a/man/check_modules_datanames.Rd
+++ b/man/check_modules_datanames.Rd
@@ -2,11 +2,14 @@
 % Please edit documentation in R/utils.R
 \name{check_modules_datanames}
 \alias{check_modules_datanames}
+\alias{check_reserved_datanames}
 \alias{check_modules_datanames_html}
 \title{Check \code{datanames} in modules}
 \usage{
 check_modules_datanames(modules, datanames)
 
+check_reserved_datanames(datanames)
+
 check_modules_datanames_html(modules, datanames)
 }
 \arguments{
diff --git a/man/pluralize.Rd b/man/pluralize.Rd
new file mode 100644
index 0000000000..eac6b67321
--- /dev/null
+++ b/man/pluralize.Rd
@@ -0,0 +1,23 @@
+% Generated by roxygen2: do not edit by hand
+% Please edit documentation in R/utils.R
+\name{pluralize}
+\alias{pluralize}
+\title{Pluralize a word depending on the size of the input}
+\usage{
+pluralize(x, singular, plural = NULL)
+}
+\arguments{
+\item{x}{(\code{object}) to check length for plural.}
+
+\item{singular}{(\code{character}) singular form of the word.}
+
+\item{plural}{(optional \code{character}) plural form of the word. If not given an "s"
+is added to the singular form.}
+}
+\value{
+A \code{character} that correctly represents the size of the \code{x} argument.
+}
+\description{
+Pluralize a word depending on the size of the input
+}
+\keyword{internal}
diff --git a/man/teal_data_module.Rd b/man/teal_data_module.Rd
index 9c3cbd29f9..9765ce4504 100644
--- a/man/teal_data_module.Rd
+++ b/man/teal_data_module.Rd
@@ -35,7 +35,9 @@ App user will be able to interact and change the data output from the module mul
 
 \item{object}{(\code{teal_data_module})}
 
-\item{code}{(\code{character} or \code{language}) code to evaluate. If \code{character}, comments are retained.}
+\item{code}{(\code{character}, \code{language} or \code{expression}) code to evaluate.
+It is possible to preserve original formatting of the \code{code} by providing a \code{character} or an
+\code{expression} being a result of \code{parse(keep.source = TRUE)}.}
 
 \item{data}{(\code{teal_data_module}) object}
 
diff --git a/tests/testthat/test-module_teal.R b/tests/testthat/test-module_teal.R
index 2e91a13f57..1c8cd82379 100644
--- a/tests/testthat/test-module_teal.R
+++ b/tests/testthat/test-module_teal.R
@@ -545,7 +545,35 @@ testthat::describe("srv_teal teal_modules", {
     )
   })
 
+
   testthat::describe("warnings on missing datanames", {
+    testthat::it("warns when dataname is not available", {
+      testthat::skip_if_not_installed("rvest")
+      shiny::testServer(
+        app = srv_teal,
+        args = list(
+          id = "test",
+          data = teal_data(iris = iris, all = mtcars),
+          modules = modules(
+            module("module_1", server = function(id, data) data)
+          )
+        ),
+        expr = {
+          session$setInputs("teal_modules-active_tab" = "module_1")
+          testthat::expect_equal(
+            trimws(
+              rvest::html_text2(
+                rvest::read_html(
+                  output[["teal_modules-module_1-validate_datanames-shiny_warnings-message"]]$html
+                )
+              )
+            ),
+            "all is reserved for internal use. Please avoid using it as a dataset name."
+          )
+        }
+      )
+    })
+
     testthat::it("warns when dataname is not available", {
       testthat::skip_if_not_installed("rvest")
       shiny::testServer(

From a50feea1e7e30cff802f47a11c3e44be79b72537 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Ver=C3=ADssimo?=
 <211358+averissimo@users.noreply.github.com>
Date: Fri, 15 Nov 2024 12:40:17 +0100
Subject: [PATCH 2/9] revert: move reserved datanames back to function

---
 R/utils.R | 2 +-
 R/zzz.R   | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/R/utils.R b/R/utils.R
index b2890c262c..7a6d85b3aa 100644
--- a/R/utils.R
+++ b/R/utils.R
@@ -135,7 +135,7 @@ check_modules_datanames <- function(modules, datanames) {
 
 #' @rdname check_modules_datanames
 check_reserved_datanames <- function(datanames) {
-  reserved_datanames <- datanames[datanames %in% getOption("teal.reserved_datanames")]
+  reserved_datanames <- datanames[datanames %in% c("all", ".raw_data")]
   if (length(reserved_datanames) == 0L) {
     return(NULL)
   }
diff --git a/R/zzz.R b/R/zzz.R
index a2cba1b4d2..62c8029561 100644
--- a/R/zzz.R
+++ b/R/zzz.R
@@ -4,7 +4,6 @@
   teal_default_options <- list(
     teal.show_js_log = FALSE,
     teal.lockfile.mode = "auto",
-    teal.reserved_datanames = c("all", ".raw_data"),
     shiny.sanitize.errors = FALSE
   )
 

From 01388eb2f1f164461305e7876935887ba0c1c82f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Ver=C3=ADssimo?=
 <211358+averissimo@users.noreply.github.com>
Date: Fri, 15 Nov 2024 12:42:35 +0100
Subject: [PATCH 3/9] docs: update news file

---
 NEWS.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/NEWS.md b/NEWS.md
index 1401db5eae..4e1871957a 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -8,6 +8,7 @@
 * Possibility to download lockfile to restore app session for reproducibility. #479
 * Introduced a function `set_datanames()` to change a `datanames` of the `teal_module`.
 * Datasets which name starts with `.` are ignored when `module`'s `datanames` is set as `"all"`.
+* Added warning when reserved `datanames`, such as `all` and `.raw_data` are being used. 
 
 ### Breaking changes
 

From 60c2badb511e6357ffd30fc009a385bbacf5e1b2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Ver=C3=ADssimo?=
 <211358+averissimo@users.noreply.github.com>
Date: Fri, 15 Nov 2024 14:41:27 +0100
Subject: [PATCH 4/9] fix: warning message isn't cleaned if not inside tag.list

---
 R/utils.R | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/R/utils.R b/R/utils.R
index 7a6d85b3aa..8a0d7540c6 100644
--- a/R/utils.R
+++ b/R/utils.R
@@ -162,7 +162,7 @@ check_modules_datanames_html <- function(modules, datanames) {
     out <- if (is.null(reserved_datanames)) {
       TRUE
     } else {
-      reserved_datanames
+      shiny::tagList(reserved_datanames)
     }
     return(out)
   }

From 95593c04d1fa0eab2ad878f383b0ccd23bcd59a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Ver=C3=ADssimo?=
 <211358+averissimo@users.noreply.github.com>
Date: Fri, 15 Nov 2024 15:01:34 +0100
Subject: [PATCH 5/9] tests: improve tests coverage by adding .raw_data and
 multiple reserved datanames

---
 tests/testthat/test-module_teal.R | 47 +++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/tests/testthat/test-module_teal.R b/tests/testthat/test-module_teal.R
index 1c8cd82379..c8a530d719 100644
--- a/tests/testthat/test-module_teal.R
+++ b/tests/testthat/test-module_teal.R
@@ -545,15 +545,54 @@ testthat::describe("srv_teal teal_modules", {
     )
   })
 
+  testthat::describe("reserved dataname is being used:", {
+    # Shared common code for tests
+    td <- within(teal.data::teal_data(), {
+      all <- mtcars
+      iris <- iris
+    })
 
-  testthat::describe("warnings on missing datanames", {
-    testthat::it("warns when dataname is not available", {
+    testthat::it("multiple datanames with `all` and `.raw_data`", {
+      testthat::skip_if_not_installed("rvest")
+
+      td_local <- within(td, {
+        .raw_data <- data.frame(
+          Species = c("Setosa", "Virginica", "Versicolor"),
+          New.Column = c("Setosas are cool", "Virginicas are also cool", "Versicolors are cool too")
+        )
+      })
+      teal.data::join_keys(td) <- teal.data::join_keys(join_key(".raw_data", "iris", "Species"))
+
+      shiny::testServer(
+        app = srv_teal,
+        args = list(
+          id = "test",
+          data = td,
+          modules = modules(module("module_1", server = function(id, data) data))
+        ),
+        expr = {
+          session$setInputs("teal_modules-active_tab" = "module_1")
+          testthat::expect_equal(
+            trimws(
+              rvest::html_text2(
+                rvest::read_html(
+                  output[["teal_modules-module_1-validate_datanames-shiny_warnings-message"]]$html
+                )
+              )
+            ),
+            "all and .raw_data are reserved for internal use. Please avoid using them as dataset names."
+          )
+        }
+      )
+    })
+
+    testthat::it("single dataname with `all`", {
       testthat::skip_if_not_installed("rvest")
       shiny::testServer(
         app = srv_teal,
         args = list(
           id = "test",
-          data = teal_data(iris = iris, all = mtcars),
+          data = within(td, all$new.column <- 1),
           modules = modules(
             module("module_1", server = function(id, data) data)
           )
@@ -573,7 +612,9 @@ testthat::describe("srv_teal teal_modules", {
         }
       )
     })
+  })
 
+  testthat::describe("warnings on missing datanames", {
     testthat::it("warns when dataname is not available", {
       testthat::skip_if_not_installed("rvest")
       shiny::testServer(

From 6b2fc08ef3539cfa6b48e7da3acddc19eb381f26 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Ver=C3=ADssimo?=
 <211358+averissimo@users.noreply.github.com>
Date: Fri, 15 Nov 2024 15:03:01 +0100
Subject: [PATCH 6/9] feat: use pluralize function on other utils.R code

---
 R/utils.R | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/R/utils.R b/R/utils.R
index 8a0d7540c6..5bd81b7a28 100644
--- a/R/utils.R
+++ b/R/utils.R
@@ -173,18 +173,19 @@ check_modules_datanames_html <- function(modules, datanames) {
       function(mod) {
         tagList(
           tags$span(
-            tags$span(if (length(mod$missing_datanames) == 1) "Dataset" else "Datasets"),
+            tags$span(pluralize(mod$missing_datanames, "Dataset")),
             to_html_code_list(mod$missing_datanames),
             tags$span(
-              paste0(
-                if (length(mod$missing_datanames) > 1) "are missing" else "is missing",
-                if (show_module_info) sprintf(" for module '%s'.", mod$label) else "."
+              sprintf(
+                "%s missing%s.",
+                pluralize(mod$missing_datanames, "is", "are"),
+                if (show_module_info) sprintf(" for module '%s'", mod$label) else ""
               )
             )
           ),
           if (length(datanames) >= 1) {
             tagList(
-              tags$span(if (length(datanames) == 1) "Dataset" else "Datasets"),
+              tags$span(pluralize(datanames, "Dataset")),
               tags$span("available in data:"),
               tagList(
                 tags$span(
@@ -408,7 +409,7 @@ paste_datanames_character <- function(x,
       tagList(
         tags$code(x[.ix]),
         if (.ix != length(x)) {
-          tags$span(ifelse(.ix == length(x) - 1, " and ", ", "))
+          tags$span(if (.ix == length(x) - 1) " and " else ", ")
         }
       )
     })
@@ -426,17 +427,18 @@ build_datanames_error_message <- function(label = NULL,
                                           tags = list(span = shiny::tags$span, code = shiny::tags$code),
                                           tagList = shiny::tagList) { # nolint: object_name.
   tags$span(
-    tags$span(ifelse(length(extra_datanames) > 1, "Datasets", "Dataset")),
+    tags$span(pluralize(extra_datanames, "Dataset")),
     paste_datanames_character(extra_datanames, tags, tagList),
     tags$span(
-      paste0(
-        ifelse(length(extra_datanames) > 1, "are missing", "is missing"),
-        ifelse(is.null(label), ".", sprintf(" for tab '%s'.", label))
+      sprintf(
+        "%s missing%s",
+        pluralize(extra_datanames, "is", "are"),
+        if (is.null(label)) "" else sprintf(" for tab '%s'", label)
       )
     ),
     if (length(datanames) >= 1) {
       tagList(
-        tags$span(ifelse(length(datanames) > 1, "Datasets", "Dataset")),
+        tags$span(pluralize(datanames, "Dataset")),
         tags$span("available in data:"),
         tagList(
           tags$span(

From c49b628b6e552aa10d9708e4e4480e79645bd221 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Ver=C3=ADssimo?=
 <211358+averissimo@users.noreply.github.com>
Date: Fri, 15 Nov 2024 15:21:02 +0100
Subject: [PATCH 7/9] fix: move teal_data code inside it

---
 tests/testthat/test-module_teal.R | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/tests/testthat/test-module_teal.R b/tests/testthat/test-module_teal.R
index c8a530d719..849b0ecd7f 100644
--- a/tests/testthat/test-module_teal.R
+++ b/tests/testthat/test-module_teal.R
@@ -546,16 +546,13 @@ testthat::describe("srv_teal teal_modules", {
   })
 
   testthat::describe("reserved dataname is being used:", {
-    # Shared common code for tests
-    td <- within(teal.data::teal_data(), {
-      all <- mtcars
-      iris <- iris
-    })
-
     testthat::it("multiple datanames with `all` and `.raw_data`", {
       testthat::skip_if_not_installed("rvest")
 
-      td_local <- within(td, {
+      # Shared common code for tests
+      td <- within(teal.data::teal_data(), {
+        all <- mtcars
+        iris <- iris
         .raw_data <- data.frame(
           Species = c("Setosa", "Virginica", "Versicolor"),
           New.Column = c("Setosas are cool", "Virginicas are also cool", "Versicolors are cool too")
@@ -588,11 +585,17 @@ testthat::describe("srv_teal teal_modules", {
 
     testthat::it("single dataname with `all`", {
       testthat::skip_if_not_installed("rvest")
+
+      td <- within(teal.data::teal_data(), {
+        all <- mtcars
+        iris <- iris
+      })
+
       shiny::testServer(
         app = srv_teal,
         args = list(
           id = "test",
-          data = within(td, all$new.column <- 1),
+          data = td,
           modules = modules(
             module("module_1", server = function(id, data) data)
           )

From 073f3b9e861c1130f1e3ec9a16903ef9d9dd3bfe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Ver=C3=ADssimo?=
 <211358+averissimo@users.noreply.github.com>
Date: Fri, 15 Nov 2024 15:26:57 +0100
Subject: [PATCH 8/9] test: add test for console warning on reserved names

---
 tests/testthat/test-init.R | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tests/testthat/test-init.R b/tests/testthat/test-init.R
index 1ebca65fa2..6619d6743d 100644
--- a/tests/testthat/test-init.R
+++ b/tests/testthat/test-init.R
@@ -108,6 +108,19 @@ testthat::test_that(
   }
 )
 
+testthat::test_that(
+  "init throws warning when datanames in modules has reserved name",
+  {
+    testthat::expect_warning(
+      init(
+        data = teal.data::teal_data(all = mtcars),
+        modules = list(example_module())
+      ),
+      "`all` is reserved for internal use\\. Please avoid using it as a dataset name\\."
+    )
+  }
+)
+
 testthat::test_that("init throws when dataname in filter incompatible w/ datanames in data", {
   testthat::expect_warning(
     init(

From 300b789f35cde6c13ded670ef4f405ec8b95f379 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Ver=C3=ADssimo?=
 <211358+averissimo@users.noreply.github.com>
Date: Fri, 15 Nov 2024 15:35:34 +0100
Subject: [PATCH 9/9] tests: group together new tests under describe/it

---
 tests/testthat/test-init.R | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/tests/testthat/test-init.R b/tests/testthat/test-init.R
index 6619d6743d..62284d1d9b 100644
--- a/tests/testthat/test-init.R
+++ b/tests/testthat/test-init.R
@@ -108,9 +108,8 @@ testthat::test_that(
   }
 )
 
-testthat::test_that(
-  "init throws warning when datanames in modules has reserved name",
-  {
+testthat::describe("init throws warning when datanames in modules has reserved name", {
+  testthat::it("`all`", {
     testthat::expect_warning(
       init(
         data = teal.data::teal_data(all = mtcars),
@@ -118,8 +117,23 @@ testthat::test_that(
       ),
       "`all` is reserved for internal use\\. Please avoid using it as a dataset name\\."
     )
-  }
-)
+  })
+
+  testthat::it("`.raw_data` and `all`", {
+    td <-
+      testthat::expect_warning(
+        init(
+          data = teal.data::teal_data(
+            all = mtcars,
+            .raw_data = iris,
+            join_keys = teal.data::join_keys(teal.data::join_key(".raw_data", "all", "a_key"))
+          ),
+          modules = list(example_module())
+        ),
+        "`.raw_data` and `all` are reserved for internal use\\. Please avoid using them as dataset names\\."
+      )
+  })
+})
 
 testthat::test_that("init throws when dataname in filter incompatible w/ datanames in data", {
   testthat::expect_warning(