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

Improves datanames error message #1297

Merged
merged 11 commits into from
Aug 12, 2024

Conversation

averissimo
Copy link
Contributor

@averissimo averissimo commented Aug 7, 2024

Part of #1253

Changes description

  • check_modules_datanames returns a string and HTML generator for:
    • string: to be used with logger
    • HTML: function to be used in teal UI
  • Message is generated in the same way. This adds complexity, but is consistent
    • c("one", "two", "three") renders as "one, two and three" (note the comma and and)
  • In the module context it doesn't show the current module label
Sample app
options(
  teal.log_level = "TRACE",
  teal.show_js_log = TRUE,
  # teal.bs_theme = bslib::bs_theme(version = 5),
  shiny.bookmarkStore = "server"
)

pkgload::load_all("teal.data")
pkgload::load_all("teal.slice")
pkgload::load_all("teal")

my_transformers <- list(
  teal_transform_module(
    label = "reactive ADSL",
    ui = function(id) {
      ns <- NS(id)
      tagList(
        div("Some UI for transform (merge)"),
        actionButton(ns("btn"), "Reload data")
      )
    },
    server = function(id, data) {
      moduleServer(id, function(input, output, session) {
        eventReactive(input$btn, {
          data()
        })
      })
    }
  ),
  teal_transform_module(
    label = "Keep first 6 from IRIS",
    ui = function(id) {
      ns <- NS(id)
      div(
        span("Some UI for transform (1)"),
        textInput(ns("obs"), label = "Number of rows", value = 6)
      )
    },
    server = function(id, data) {
      moduleServer(id, function(input, output, session) {
        reactive({
          req(data())
          obs <- as.numeric(input$obs)
          if (!is.finite(obs)) stop("NOT NUMERIC.")
          within(data(), iris <- head(iris, n), n = as.numeric(input$obs))
        })
      })
    }
  ),
  teal_transform_module(
    label = "Keep first 6 from ADTTE",
    ui = function(id) div("Some UI for transform 2"),
    server = function(id, data) {
      moduleServer(id, function(input, output, session) {
        reactive({
          req(data())
          within(data(), ADTTE <- head(ADTTE))
        })
      })
    }
  )
)

data <- teal_data_module(
  once = FALSE,
  ui = function(id) {
    ns <- NS(id)
    tagList(
      numericInput(ns("obs"), "Number of observations to show", 1000),
      actionButton(ns("submit"), label = "Submit")
    )
  },
  server = function(id, ...) {
    moduleServer(id, function(input, output, session) {
      logger::log_trace("example_module_transform2 initializing.")
      eventReactive(input$submit, {
        data <- teal_data() |>
          within(
            {
              logger::log_trace("Loading data")
              ADSL <- head(teal.data::rADSL, n = n)
              ADTTE <- teal.data::rADTTE
              iris <- iris
              
              CO2 <- CO2
              factors <- names(Filter(isTRUE, vapply(CO2, is.factor, logical(1L))))
              CO2[factors] <- lapply(CO2[factors], as.character)
            },
            n = as.numeric(input$obs)
          )
        join_keys(data) <- default_cdisc_join_keys[c("ADSL", "ADTTE")]
        teal.data::datanames(data) <- c("ADSL", "ADTTE", "iris", "CO2")
        data
      })
    })
  }
)

teal::init(
  data = data,
  modules = list(
    example_module("mod-1", datanames = "all"),
    example_module("mod-2", transformers = my_transformers, datanames = c("ADSL", "ADTTE", "iris", "elo")),
    modules(
      label = "sub-modules",
      example_module("mod-2-sub1", transformers = my_transformers, datanames = c("ADSL", "ADTTE", "iris", "elo", "elo2")),
      example_module("mod-2-sub2", transformers = my_transformers, datanames = c("ADSL", "ADTTE", "iris", "elo"))
    ),
    example_module("mod-2", transformers = my_transformers[2:3])
  ),
  filter = teal_slices(
    teal_slice("ADSL", "SEX"),
    teal_slice("ADSL", "AGE", selected = c(18L, 65L))
  )
) |>
  runApp()

image

image

@averissimo averissimo added the core label Aug 7, 2024
@averissimo averissimo mentioned this pull request Aug 8, 2024
63 tasks
…s_error@669_insertUI@main

* origin/669_insertUI@main:
  [skip style] [skip vbump] Restyle files
  fix: silent error showing in console
  restart cicd
@m7pr
Copy link
Contributor

m7pr commented Aug 9, 2024

Hey @averissimo there is a conflict in R/module_data_summary.R I think there is difference in this line

req(!inherits(attr(summary_table_out, "condition"), "shiny.silent.error"))

…s_error@669_insertUI@main

* origin/669_insertUI@main:
  tests: update tests with modified transform code
  tests: add test for expected behavior on fallback and recovery
  fix: trigger .fallback when upstream data changes
  fix: correct the broken test logic
  chore: fix spellcheck
  [skip roxygen] [skip vbump] Roxygen Man Pages Auto Update
  [skip style] [skip vbump] Restyle files
  Exclude the "_raw" datasets only if the original dataset exists (#1305)
@averissimo
Copy link
Contributor Author

For users, the datanames concept is not obvious, suggesting using Datasets in message. See 2 screenshots below

image

image

@averissimo
Copy link
Contributor Author

@m7pr thanks!

list(
string = build_datanames_error_message(
modules$label,
datanames,
Copy link
Contributor

Choose a reason for hiding this comment

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

We enumerate all available datanames in the included object even those irrelevant for the module. If mod-2 uses only c("iris", "inexisting"), then listing ADSL, ADTTE, etc. seems irrelevant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see it as a helpuful debugging information when the app developer is designing the app, but it will be irrelevant for normal users. So, does it:

  1. Makes sense to keep a second part of the message?
  2. Or should we keep the valid datanames and say: "iris is available" (for your example)

Copy link
Contributor

@gogonzo gogonzo Aug 12, 2024

Choose a reason for hiding this comment

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

I'm fine with both as far as (1) will be rephrased little bit. available data for 'mod-2', which has: . It is not straightforward what "which has" is exactly pointing to, module or data?
I'd prefer: Dataset(s) ... is/are missing for tab 'mod-2'. Datasets available in data: ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, here's a new swing with that suggestion, WDYT?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added messsage exception when there are no datasets available:

image

No datasets available example
options(
  teal.log_level = "TRACE",
  teal.show_js_log = TRUE,
  # teal.bs_theme = bslib::bs_theme(version = 5),
  shiny.bookmarkStore = "server"
)

pkgload::load_all("teal.data")
pkgload::load_all("teal.slice")
pkgload::load_all("teal")

my_transformers <- list(
  teal_transform_module(
    label = "Add ELO",
    ui = function(id) {
      ns <- NS(id)
      tagList(
        div("Some UI for transform (merge)"),
        actionButton(ns("btn"), "Reload data")
      )
    },
    server = function(id, data) {
      moduleServer(id, function(input, output, session) {
        eventReactive(input$btn, {
          data() |> within(mtcars <- datasets::mtcars)
        })
      })
    }
  )
)

teal::init(
  data = teal.data::teal_data(iris = iris),
  modules = list(
    example_module("mod-1", datanames = c("mtcars"), transformers = my_transformers),
    example_module("mod-2", datanames = c("iris"))
  )
) |>
  runApp()

@gogonzo gogonzo self-assigned this Aug 12, 2024
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

Spennande!

@averissimo averissimo merged commit a6b21f8 into 669_insertUI@main Aug 12, 2024
1 check passed
@averissimo averissimo deleted the datanames_error@669_insertUI@main branch August 12, 2024 10:59
@github-actions github-actions bot locked and limited conversation to collaborators Aug 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants