Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

736 Allow pure custom card functions in modules #742

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

chlebowa
Copy link
Contributor

@chlebowa chlebowa commented Apr 23, 2024

Solves #736.
Alternative to #737.
Accompanied by insightsengineering/teal.reporter#270.
Allows custom card functions to be pure functions.

Card functions must receive an environment in argument env, which contains all elements to which the card function will refer when building a report card.

A card function will now look like this (note the env$):

local_card_function <- function(comment, label, env) {
  card <- teal::report_card_template(
    title = "Distribution Plot",
    label = label,
    with_filter = env$with_filter,              # env$ here
    filter_panel_api = env$filter_panel_api     # env$ here
  )
  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
}

@chlebowa chlebowa changed the title Allow pure custom card functions in modules 736 Allow pure custom card functions in modules Apr 24, 2024
Copy link
Contributor

github-actions bot commented May 2, 2024

CLA Assistant Lite bot ✅ All contributors have signed the CLA

@chlebowa
Copy link
Contributor Author

chlebowa commented May 2, 2024

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

}
###
})
}

#' @keywords internal
tm_g_distribution_card_function <- function(comment, label, env) { # nolint: object_length.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is something we need to rethink (the way how to handle custom reporting)


I think there are two ways to handle this:

  1. One you described above where custom function passed through argument is executed in the srv environment, or
  2. module returns reporter card (and other elements) and one can wrap function around srv to bypass and edit reporter card on the fly. (I guess this is not possible now and require some refactoring)

(1) is related with the idea we are currently exploring which is called "preprocessing", where app developer can pass any code/function to the srv and "postprocess" existing items in that environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me the solution you are proposing involves a significant rewrite of the modules as well as the framework. In the long run it is probably superior but I imagine it would take time to properly design and implement.
The solution proposed here is quicker to implement and introduces the feature of custom reporting without changing the framework in a meaningful way. It can be used as a temporary measure. I imagine that if a module returns something (be it a TealReportCard object or a list of its possible contents), the way to specify its handling would be to pass a function.

I will be happy to discuss further.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @chlebowa that this is temporary solution that is quick an easy to apply. We can have a broader discussion on how to improve that in the future, but I think for now this is ready to go and can be extended to other modules (and other packages teal.modules.clinical, teal.osprey etc).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants