736 Allow custom card functions in modules

[chlebowa]

Solves #736

Allows app developers to pass card functions to modules as arguments.

Demonstration

Example app demonstrating use of locally defined card function:

example app

rm(list = ls())

library(teal)
devtools::load_all("./teal.modules.general")
library(teal.widgets)

local_card_function <- function(comment, label) {
  card <- teal::report_card_template(
    title = "Distribution Plot",
    label = label,
    with_filter = FALSE,
    filter_panel_api = filter_panel_api
  )
  card$append_text("Dummy Card", "header2")
  card$append_text(paste(
    "This report card was generated by a card function",
    "defined in the global environment."
  ))
  if (!comment == "") {
    card$append_text("Comment", "header3")
    card$append_text(comment)
  }
  card
}

data <- teal_data() |> within({
  iris <- iris
  mtcars <- mtcars
  faithful <- faithful
})
datanames(data) <- c("iris", "mtcars", "faithful")
filter <- teal_slices(
  teal_slice("iris", "Petal.Length", selected = c(1, 5.1)),
  teal_slice("iris", "Species", selected = c("setosa", "virginica")),
  teal_slice("mtcars", "cyl", selected = 6L),
  teal_slice("faithful", "waiting", selected = c(76, 82)),
  module_specific = TRUE,
  mapping = list(
    # "Distribution Module" = "iris Petal.Length",
    "Example" = "faithful waiting",
    "global_filters" = "iris Species"
  )
)
app <- init(
  data = data,
  modules = modules(
    tm_g_distribution(
      label = "Default card function",
      dist_var = data_extract_spec(
        dataname = "iris",
        select = select_spec(variable_choices("iris"), "Petal.Length")
      ),
      ggplot2_args = ggplot2_args(
        labs = list(subtitle = "Plot generated by Distribution Module")
      )
    ),
    tm_g_distribution(
      label = "Local card function",
      dist_var = data_extract_spec(
        dataname = "iris",
        select = select_spec(variable_choices("iris"), "Petal.Length")
      ),
      ggplot2_args = ggplot2_args(
        labs = list(subtitle = "Plot generated by Distribution Module")
      ),
      card_function = local_card_function
    ),
    example_module("Example")
  ),
  filter = filter
)

runApp(app, launch.browser = TRUE)

Also works with card function defined in another package (not shown).

Rationale

Currently each module has a hard-coded card function so the module developer has full control over what is added to the report. Neither the app user, nor the app developer has any influence on this.

Being able to redefine the card function is a valid use case.

Scoping Consideration

Current card functions are defined within the module server functions because they rely heavily on bindings that exist in the execution environment of the server function. In order for a function defined elsewhere to be able to access those bindings, a function is introduced, hydrate_function. hydrate_function changes the enclosing environment of a function (here, a card function) to bring another frame into scope. It can also add other bindings to the funciton's enclosure.

Implementation

• Card functions from all modules that support reporting are isolated as separate, unexported functions. The naming convention is <module_name>_card_function.
• All modules that support reporting receive a card_function formal argument. If not provided, falls back to the corresponding card function for the module.
• Within modules, the card function, whether one provided in the call or the fallback value, is passed through hydrate_function to extend its scope to include the module server function's execution environment. This call is identical in all modules:
  card_function <- hydrate_function(card_function, with_filter, filter_panel_api).

NOTE

This is a proof of concept, changes have only been made to tm_g_distribution.

Progress

• tm_a_pca.R
• tm_a_regression.R
• tm_data_table.R
• tm_g_association.R
• tm_g_bivariate.R
• tm_g_distribution.R
• tm_g_response.R
• tm_g_scatterplot.R
• tm_g_scatterplotmatrix.R
• tm_missing_data.R
• tm_outliers.R
• tm_t_crosstable.R
• tm_variable_browser.R

[pawelru]

Being able to redefine the card function is a valid use case.

I very much agree with this. I like the request and I do like the idea for implementation - that is: a card factory function. There are some details that I am still not quite sure yet and these could potentially require changes in teal.reporter itself (hopefully not but I cannot guarantee).

[pawelru]

my main concern is that this is not a true list of required arguments. We are using with_filter and filter_panel_api. Moreover, we are using dist_r(), pws1 and many more object from the parent environment.
You have addressed this with hydrate_function but I think it would be better not to be in need of such functionality. I can think of a few ways how to do this:

• extend function arguments
• pass data as environment
• this being a module?

(I really hope that there is no strict check for these and only these argument somewhere in the teal.reporter).

Each has its pros and cons and we probably need to think more which one would be best for this task. Glad you stopped early to allow for a discussion like this.

Looking at the changes - this is how it was written in the past so I definitely not blaming you for this. This PR is a great opportunity to make it right.

[chlebowa]

(I really hope that there is no strict check for these and only these argument somewhere in the teal.reporter).

There is one, actually. I considered just adding ... to the card function's formals but that is forbidden. I think it would work but I am not sure about resolving conflicts between the variables passed to the ellipsis and bindings in the card function's parent environment. If we can figure out a way for the card functions to be pure, this would not be a problem.

I tried to limit the proposed changes to the module packages because I assumed modifying teal.reporter is off the table.

Note that passing the caller environment is not sufficient, at least in this module (I assume in others as well). with_filter and filter_panel_api are also required but they exist in the caller's parent frame, not in the caller itself.

I will be happy to discuss a satisfactory solution.

---

Note also that with the proposed solution the if (with_reporter) chunk would be the same in every module, which means _all code added to the module to enable reporting _would be the same in every module. The only difference would be the default card function, which is separate.

This in turn opens another possibility: have a function called with_reporter that would modify any module object by adding the code required for reporting into the module functions. Then adding support for reporting would be limited to something like modules(tm_g_distribution(...) |> with_reporter(card_fun = foo)). But that's a separate conversation.

[pawelru]

Ahh that's bad :( Then I would say that please feel free to modify this in teal.reporter. I feel that this is the true root-cause of the issue. I also think that this (relaxing this check on formals) would make creating card-factory-fun much simpler and also encapsulated so that it can be run without any additional steps - just provide what's required.
That's exactly what I'm looking for - more simplicity. I'm a little bit afraid of introducing a requirement of hydrating a card-factory-fun. I don't think that most of our users (in particular: not developers) would handle this.

[chlebowa]

Have a look at this alternative: #742

I don't think it is full proof because of the way teal.reporter checks and handles formal arguments of card_fun:
https://github.com/insightsengineering/teal.reporter/blob/ad7fd2041f0d7cac83d0d97dbbe3ed7914a197ea/R/AddCardModule.R#L166

Note that environment shenanigans are already built into teal.reporter:
https://github.com/insightsengineering/teal.reporter/blob/ad7fd2041f0d7cac83d0d97dbbe3ed7914a197ea/R/AddCardModule.R#L173

EDIT: I had got a little bit of tunnel vision, with an added env argument wrapping the card function is not necessary. The alternative is not that bad now. Card functions are pure, though it may be tedious to reference env$ for all added bindings.

[github-actions]

CLA Assistant Lite bot ✅ All contributors have signed the CLA

[chlebowa]

I have read the CLA Document and I hereby sign the CLA

[m7pr]

Suggested change

logger (>= 0.3.0),

[m7pr]

As rlang was introduced as a dependency, it also needs to be added here and in .pre-commit-config.yaml

[m7pr]

Suggested change

	#' This allows any funciton to use bindings present in any environment
	#' This allows any function to use bindings present in any environment

[m7pr]

Suggested change

	#' as if the funciton were defined there.
	#' as if the function were defined there.

[m7pr]

Suggested change

	#' by the module server function so the server funciton's arguments are in scope
	#' by the module server function so the server function's arguments are in scope