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.

Sign up for GitHub

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
Merged

Improves datanames error message #1297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
880e16e
feat: improve error message for datanames
averissimo Aug 7, 2024
7fa997d
Merge remote-tracking branch 'origin/669_insertUI@main' into dataname…
averissimo Aug 8, 2024
45bda47
style: adds padding to icon on error message
averissimo Aug 9, 2024
c586ba7
Merge remote-tracking branch 'origin/669_insertUI@main' into dataname…
averissimo Aug 9, 2024
a38370d
feat: change phrase to be easier for users to understand
averissimo Aug 9, 2024
42c7ad8
docs: another small adjustment
averissimo Aug 9, 2024
3ff20d5
docs: another small adjustment
averissimo Aug 9, 2024
2a40d69
Merge branch '669_insertUI@main' into datanames_error@669_insertUI@main
averissimo Aug 12, 2024
037a6a5
docs: feedback from PR to change message
averissimo Aug 12, 2024
581d0f5
fix: error message when there are no datanames available in data
averissimo Aug 12, 2024
b12eaae
Merge branch '669_insertUI@main' into datanames_error@669_insertUI@main
averissimo Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion R/init.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ init <- function(data,

is_modules_ok <- check_modules_datanames(modules, .teal_data_datanames(data))
if (!isTRUE(is_modules_ok)) {
logger::log_warn(is_modules_ok)
lapply(is_modules_ok$string, logger::log_warn)
}

is_filter_ok <- check_filter_datanames(filter, .teal_data_datanames(data))
Expand Down
9 changes: 7 additions & 2 deletions R/module_teal_data.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@ srv_validate_reactive_teal_data <- function(id, # nolint: object_length
)
)


data_out
})

Expand All @@ -164,7 +163,13 @@ srv_validate_reactive_teal_data <- function(id, # nolint: object_length
if (inherits(data_out_rv(), "teal_data")) {
is_modules_ok <- check_modules_datanames(modules = modules, datanames = .teal_data_ls(data_validated()))
if (!isTRUE(is_modules_ok)) {
tags$div(is_modules_ok, class = "teal-output-warning")
tags$div(
is_modules_ok$html(
# Show modules prefix on message only in teal_data_module tab
grepl(sprintf("data-teal_data_module-%s", id), session$ns(NULL), fixed = TRUE)
),
class = "teal-output-warning"
)
}
}
})
Expand Down
94 changes: 86 additions & 8 deletions R/utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,22 +119,48 @@ check_modules_datanames <- function(modules, datanames) {
recursive_check_datanames <- function(modules, datanames) {
# check teal_modules against datanames
if (inherits(modules, "teal_modules")) {
sapply(modules$children, function(module) recursive_check_datanames(module, datanames = datanames))
result <- lapply(modules$children, function(module) recursive_check_datanames(module, datanames = datanames))
result <- result[vapply(result, Negate(is.null), logical(1L))]
list(
string = do.call(c, as.list(unname(sapply(result, function(x) x$string)))),
html = function(with_module_name = TRUE) {
tagList(
lapply(
result,
function(x) x$html(with_module_name = with_module_name)
)
)
}
)
} else {
extra_datanames <- setdiff(modules$datanames, c("all", datanames))
if (length(extra_datanames)) {
sprintf(
"Module '%s' uses datanames not available in 'data': (%s) not in (%s)",
modules$label,
toString(dQuote(extra_datanames, q = FALSE)),
toString(dQuote(datanames, q = FALSE))
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 •
edited
Loading

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()

extra_datanames,
tags = list(span = paste, code = function(x) toString(dQuote(x, q = FALSE))),
paste0
),
# Build HTML representation of the error message with <pre> formatting
html = function(with_module_name = TRUE) {
tagList(
build_datanames_error_message(
if (with_module_name) modules$label,
datanames,
extra_datanames
),
tags$br(.noWS = "before")
)
}
)
}
}
}
check_datanames <- unlist(recursive_check_datanames(modules, datanames))
check_datanames <- recursive_check_datanames(modules, datanames)
if (length(check_datanames)) {
paste(check_datanames, collapse = "\n")
check_datanames
} else {
TRUE
}
Expand Down Expand Up @@ -288,3 +314,55 @@ strip_style <- function(string) {
useBytes = TRUE
)
}

#' Convert character list to human readable html with commas and "and"
#' @noRd
paste_datanames_character <- function(x,
tags = list(span = shiny::tags$span, code = shiny::tags$code),
tagList = shiny::tagList) {
checkmate::assert_character(x)
do.call(
tagList,
lapply(seq_along(x), function(.ix) {
tagList(
tags$code(x[.ix]),
if (.ix != length(x)) {
tags$span(ifelse(.ix == length(x) - 1, " and ", ", "))
}
)
})
)
}

#' Build datanames error string for error message
#'
#' tags and tagList are overwritten in arguments allowing to create strings for
#' logging purposes
#' @noRd
build_datanames_error_message <- function(label = NULL,
datanames,
extra_datanames,
tags = list(span = shiny::tags$span, code = shiny::tags$code),
tagList = shiny::tagList) {
tags$span(
tags$span(ifelse(length(extra_datanames) > 1, "Datasets", "Dataset")),
paste_datanames_character(extra_datanames, tags, tagList),
tags$span(ifelse(length(extra_datanames) > 1, "are missing", "is missing")),
tags$span(
paste(
ifelse(is.null(label), ".", sprintf("for tab '%s'.", label))
),
.noWS = c("before")
),
if (length(datanames) >= 1) {
tagList(
tags$span(ifelse(length(datanames) > 1, "Datasets", "Dataset")),
tags$span("available in data:"),
tags$span(paste_datanames_character(datanames, tags, tagList)),
tags$span(".", .noWS = "before")
)
} else {
tags$span("No datasets are available in data.")
}
)
}
11 changes: 6 additions & 5 deletions inst/css/validation.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,24 @@

.teal_validated .teal-output-warning::before {
content: "\26A0\FE0F";
padding-right: 0.5em;
}

.teal_validated .shiny-output-error::before {
content: "\1F6A8";
padding-right: 0.5em;
}

.teal_primary_col .teal-output-warning::before {
.teal_primary_col .shiny-output-error::before {
content: "\1F6A8";
padding-right: 0.5em;
}

.teal_primary_col .teal-output-warning::before {
content: "\26A0\FE0F";
}

.teal_primary_col .teal_validated:has(.shiny-output-error),
.teal_primary_col .teal_validated:has(.teal-output-warning) {
margin: 1em 0 1em 0;
padding: .5em 0 .5em 0;
padding: .5em 0 .5em .5em;
}

.teal_primary_col > .teal_validated:has(.teal-output-warning),
Expand Down
Loading