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

Ensures required packages are in Imports section #774

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
+122 −173
Open

Ensures required packages are in Imports section #774

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e2275ab
chore: move required packages to imports
averissimo Aug 14, 2024
4590f44
fix: ensure optional requirements in tests
averissimo Aug 14, 2024
8484c82
Merge branch 'main' into 518-pkgs_imports@main
averissimo Dec 18, 2024
a87ccd1
chore: make sure all github are included
averissimo Dec 18, 2024
3015749
chore: remove require namespaces from examples
averissimo Dec 18, 2024
b083bc4
chore: remove whitespace
averissimo Dec 18, 2024
fcedb3c
chore: remove unecessary skips
averissimo Dec 18, 2024
98ae21c
[skip roxygen] [skip vbump] Roxygen Man Pages Auto Update
github-actions[bot] Dec 18, 2024
27a75ad
empty: trigger ci
averissimo Dec 19, 2024
8d1c172
test: action with custom max additional repos
averissimo Dec 20, 2024
ad8ba68
Revert "test: action with custom max additional repos"
averissimo Dec 20, 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
35 changes: 17 additions & 18 deletions DESCRIPTION
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I see changes in dependencies and I immediately think about verdepcheck. Can you please confirm that it is passing on this? If not - let's work on the deps versions.

This might need to wait for https://github.com/insightsengineering/teal.modules.general/pull/774/files#r1890465398

Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,29 @@ Depends:
teal.transform (>= 0.5.0)
Imports:
checkmate (>= 2.1.0),
colourpicker,
dplyr (>= 1.0.5),
DT (>= 0.13),
forcats (>= 1.0.0),
generics,
ggExtra,
ggpmisc (>= 0.4.3),
ggpp,
ggrepel,
goftest,
grid,
gridExtra,
htmlwidgets,
jsonlite,
lattice (>= 0.18-4),
MASS,
rtables (>= 0.6.8),
llrs-roche marked this conversation as resolved.
Show resolved Hide resolved
scales,
shinyjs,
shinyTree (>= 0.2.8),
shinyvalidate,
shinyWidgets (>= 0.5.1),
sparkline,
stats,
stringr (>= 1.4.1),
teal.code (>= 0.5.0),
Expand All @@ -53,28 +67,14 @@ Imports:
tools,
utils
Suggests:
broom (>= 0.7.10),
colourpicker,
ggExtra,
ggpmisc (>= 0.4.3),
ggpp,
ggrepel,
goftest,
gridExtra,
htmlwidgets,
jsonlite,
knitr (>= 1.42),
lattice (>= 0.18-4),
logger (>= 0.2.0),
MASS,
nestcolor (>= 0.1.0),
pkgload,
rlang (>= 1.0.0),
rmarkdown (>= 2.23),
rtables (>= 0.6.8),
rvest,
shinytest2,
sparkline,
testthat (>= 3.1.9),
withr (>= 2.0.0)
VignetteBuilder:
Expand All @@ -88,10 +88,9 @@ Config/Needs/verdepcheck: haleyjeppson/ggmosaic, tidyverse/ggplot2,
insightsengineering/teal.data, insightsengineering/teal.logger,
insightsengineering/teal.reporter, insightsengineering/teal.widgets,
insightsengineering/tern, tidyverse/tibble, tidyverse/tidyr,
tidymodels/broom, daattali/colourpicker, daattali/ggExtra,
aphalo/ggpmisc, aphalo/ggpp, slowkow/ggrepel, baddstats/goftest,
gridExtra, ramnathv/htmlwidgets, jeroen/jsonlite, yihui/knitr,
daroczig/logger, deepayan/lattice, MASS,
daattali/colourpicker, daattali/ggExtra, aphalo/ggpmisc, aphalo/ggpp,
slowkow/ggrepel, baddstats/goftest, gridExtra, ramnathv/htmlwidgets,
jeroen/jsonlite, yihui/knitr, daroczig/logger, deepayan/lattice, MASS,
insightsengineering/nestcolor, r-lib/rlang, rstudio/rmarkdown,
insightsengineering/rtables, tidyverse/rvest, sparkline,
rstudio/shinytest2, r-lib/testthat, r-lib/withr
Expand Down
14 changes: 2 additions & 12 deletions R/tm_g_distribution.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,16 +121,6 @@ tm_g_distribution <- function(label = "Distribution Module",
post_output = NULL) {
message("Initializing tm_g_distribution")

# Requires Suggested packages
extra_packages <- c("ggpmisc", "ggpp", "goftest", "MASS", "broom")
missing_packages <- Filter(function(x) !requireNamespace(x, quietly = TRUE), extra_packages)
if (length(missing_packages) > 0L) {
stop(sprintf(
"Cannot load package(s): %s.\nInstall or restart your session.",
toString(missing_packages)
))
}

# Normalize the parameters
if (inherits(dist_var, "data_extract_spec")) dist_var <- list(dist_var)
if (inherits(strata_var, "data_extract_spec")) strata_var <- list(strata_var)
Expand Down Expand Up @@ -1176,7 +1166,7 @@ srv_distribution <- function(id,
expr = {
test_stats <- ANL %>%
dplyr::select(dist_var) %>%
with(., broom::glance(do.call(test, args))) %>%
with(., generics::glance(do.call(test, args))) %>%
dplyr::mutate_if(is.numeric, round, 3)
},
env = env
Expand All @@ -1190,7 +1180,7 @@ srv_distribution <- function(id,
test_stats <- ANL %>%
dplyr::select(dist_var, s_var, g_var) %>%
dplyr::group_by_at(dplyr::vars(dplyr::any_of(groups))) %>%
dplyr::do(tests = broom::glance(do.call(test, args))) %>%
dplyr::do(tests = generics::glance(do.call(test, args))) %>%
tidyr::unnest(tests) %>%
dplyr::mutate_if(is.numeric, round, 3)
},
Expand Down
10 changes: 0 additions & 10 deletions R/tm_g_scatterplot.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,16 +229,6 @@ tm_g_scatterplot <- function(label = "Scatterplot",
ggplot2_args = teal.widgets::ggplot2_args()) {
message("Initializing tm_g_scatterplot")

# Requires Suggested packages
extra_packages <- c("ggpmisc", "ggExtra", "colourpicker")
missing_packages <- Filter(function(x) !requireNamespace(x, quietly = TRUE), extra_packages)
if (length(missing_packages) > 0L) {
stop(sprintf(
"Cannot load package(s): %s.\nInstall or restart your session.",
toString(missing_packages)
))
}

# Normalize the parameters
if (inherits(x, "data_extract_spec")) x <- list(x)
if (inherits(y, "data_extract_spec")) y <- list(y)
Expand Down
5 changes: 0 additions & 5 deletions R/tm_g_scatterplotmatrix.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,6 @@ tm_g_scatterplotmatrix <- function(label = "Scatterplot Matrix",
post_output = NULL) {
message("Initializing tm_g_scatterplotmatrix")

# Requires Suggested packages
if (!requireNamespace("lattice", quietly = TRUE)) {
stop("Cannot load lattice - please install the package or restart your session.")
}

# Normalize the parameters
if (inherits(variables, "data_extract_spec")) variables <- list(variables)

Expand Down
88 changes: 45 additions & 43 deletions R/tm_missing_data.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,6 @@ tm_missing_data <- function(label = "Missing data",
post_output = NULL) {
message("Initializing tm_missing_data")

# Requires Suggested packages
if (!requireNamespace("gridExtra", quietly = TRUE)) {
stop("Cannot load gridExtra - please install the package or restart your session.")
}
if (!requireNamespace("rlang", quietly = TRUE)) {
stop("Cannot load rlang - please install the package or restart your session.")
}

# Normalize the parameters
if (inherits(ggplot2_args, "ggplot2_args")) ggplot2_args <- list(default = ggplot2_args)

Expand Down Expand Up @@ -1135,6 +1127,13 @@ srv_missing_data <- function(id, data, reporter, filter_panel_api, dataname, par
ggtheme = input$ggtheme
)

# Unlikely that `rlang` is not available, new hashing may be expensive
hashing_function <- if (requireNamespace("rlang", quietly = TRUE)) {
quote(rlang::hash)
} else {
function(x) paste(as.integer(x), collapse = "")
averissimo marked this conversation as resolved.
Show resolved Hide resolved
}

teal.code::eval_code(
common_code_q(),
substitute(
Expand All @@ -1149,41 +1148,44 @@ srv_missing_data <- function(id, data, reporter, filter_panel_api, dataname, par
)
) %>%
teal.code::eval_code(
quote({
summary_plot_patients <- ANL[, c(parent_keys, analysis_vars)] %>%
dplyr::group_by_at(parent_keys) %>%
dplyr::mutate(id = dplyr::cur_group_id()) %>%
dplyr::ungroup() %>%
dplyr::group_by_at(c(parent_keys, "id")) %>%
dplyr::summarise_all(anyNA) %>%
dplyr::ungroup()

# order subjects by decreasing number of missing and then by
# missingness pattern (defined using sha1)
order_subjects <- summary_plot_patients %>%
dplyr::select(-"id", -dplyr::all_of(parent_keys)) %>%
dplyr::transmute(
id = dplyr::row_number(),
number_NA = apply(., 1, sum),
sha = apply(., 1, rlang::hash)
) %>%
dplyr::arrange(dplyr::desc(number_NA), sha) %>%
getElement(name = "id")

# order columns by decreasing percent of missing values
ordered_columns <- summary_plot_patients %>%
dplyr::select(-"id", -dplyr::all_of(parent_keys)) %>%
dplyr::summarise(
column = create_cols_labels(colnames(.)),
na_count = apply(., MARGIN = 2, FUN = sum),
na_percent = na_count / nrow(.) * 100
) %>%
dplyr::arrange(na_percent, dplyr::desc(column))

summary_plot_patients <- summary_plot_patients %>%
tidyr::gather("col", "isna", -"id", -dplyr::all_of(parent_keys)) %>%
dplyr::mutate(col = create_cols_labels(col))
})
substitute(
expr = {
summary_plot_patients <- ANL[, c(parent_keys, analysis_vars)] %>%
dplyr::group_by_at(parent_keys) %>%
dplyr::mutate(id = dplyr::cur_group_id()) %>%
dplyr::ungroup() %>%
dplyr::group_by_at(c(parent_keys, "id")) %>%
dplyr::summarise_all(anyNA) %>%
dplyr::ungroup()

# order subjects by decreasing number of missing and then by
# missingness pattern (defined using sha1)
order_subjects <- summary_plot_patients %>%
dplyr::select(-"id", -dplyr::all_of(parent_keys)) %>%
dplyr::transmute(
id = dplyr::row_number(),
number_NA = apply(., 1, sum),
sha = apply(., 1, hashing_function)
) %>%
dplyr::arrange(dplyr::desc(number_NA), sha) %>%
getElement(name = "id")

# order columns by decreasing percent of missing values
ordered_columns <- summary_plot_patients %>%
dplyr::select(-"id", -dplyr::all_of(parent_keys)) %>%
dplyr::summarise(
column = create_cols_labels(colnames(.)),
na_count = apply(., MARGIN = 2, FUN = sum),
na_percent = na_count / nrow(.) * 100
) %>%
dplyr::arrange(na_percent, dplyr::desc(column))

summary_plot_patients <- summary_plot_patients %>%
tidyr::gather("col", "isna", -"id", -dplyr::all_of(parent_keys)) %>%
dplyr::mutate(col = create_cols_labels(col))
},
env = list(hashing_function = hashing_function)
)
) %>%
teal.code::eval_code(
substitute(
Expand Down
5 changes: 0 additions & 5 deletions R/tm_t_crosstable.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,6 @@ tm_t_crosstable <- function(label = "Cross Table",
basic_table_args = teal.widgets::basic_table_args()) {
message("Initializing tm_t_crosstable")

# Requires Suggested packages
if (!requireNamespace("rtables", quietly = TRUE)) {
stop("Cannot load rtables - please install the package or restart your session.")
}

# Normalize the parameters
if (inherits(x, "data_extract_spec")) x <- list(x)
if (inherits(y, "data_extract_spec")) y <- list(y)
Expand Down
12 changes: 0 additions & 12 deletions R/tm_variable_browser.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,6 @@ tm_variable_browser <- function(label = "Variable Browser",
ggplot2_args = teal.widgets::ggplot2_args()) {
message("Initializing tm_variable_browser")

# Requires Suggested packages
if (!requireNamespace("sparkline", quietly = TRUE)) {
stop("Cannot load sparkline - please install the package or restart your session.")
}
if (!requireNamespace("htmlwidgets", quietly = TRUE)) {
stop("Cannot load htmlwidgets - please install the package or restart your session.")
}
if (!requireNamespace("jsonlite", quietly = TRUE)) {
stop("Cannot load jsonlite - please install the package or restart your session.")
}

# Start of assertions
checkmate::assert_string(label)
checkmate::assert_character(datasets_selected)
Expand Down Expand Up @@ -1280,7 +1269,6 @@ create_sparklines.default <- function(arr, width = 150, ...) {
as.character(tags$code("unsupported variable type", class = "text-blue"))
}


custom_sparkline_formatter <- function(labels, counts) {
htmlwidgets::JS(
sprintf(
Expand Down
4 changes: 3 additions & 1 deletion tests/testthat/setup-logger.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
logger::log_appender(function(...) NULL, namespace = "teal.modules.general")
if (requireNamespace("logger", quietly = TRUE)) {
logger::log_appender(function(...) NULL, namespace = "teal.modules.general")
}
1 change: 1 addition & 0 deletions tests/testthat/test-examples.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ for (i in rd_files()) {
paste0("example-", basename(i)),
{
skip_if_too_deep(5)
testthat::skip_if_not_installed("pkgload")
if (basename(i) %in% strict_exceptions) {
op <- options()
withr::local_options(opts_partial_match_old)
Expand Down
1 change: 1 addition & 0 deletions tests/testthat/test-shinytest2-tm_data_table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ test_that("e2e - tm_data_table: Verify checkbox displayed over data table", {
})

test_that("e2e - tm_data_table: Verify module displays data table", {
testthat::skip_if_not_installed("rvest")
Copy link
Contributor

@pawelru pawelru Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have recently pushed changes into the teal due to noSuggest check and the class definition looks like this:
https://github.com/insightsengineering/teal/blob/7eac4d58a5373aa511f363a2f259d850749501f7/R/TealAppDriver.R#L10-L20

Just thinking loud now, would it help if we would have the following instead?

    if (!requireNamespace("shinytest2", quietly = TRUE)) {
      if (requireNamespace("testthat", quietly = TRUE) && getNamespace("testthat")$is_testing()) {  
       getNamespace("testthat")$skip("shinytest2 is not installed")
      } else {
        stop("Please install 'shinytest2' package to use this class.")
      }
    }

My motivation is to avoid a lot of skip statements in the upstream packages like here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's awesome, I'll create the PR in {teal} to do just this. I think the $is_testing() is not required as skip() throws an error and it's on testthat to capture it and handle

skip_if_too_deep(5)
app_driver <- app_driver_tm_data_table()

Expand Down
3 changes: 3 additions & 0 deletions tests/testthat/test-shinytest2-tm_file_viewer.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ app_driver_tm_file_viewer <- function() {
}

test_that("e2e - tm_file_viewer: Initializes without errors and shows files tree specified in input_path argument", {
testthat::skip_if_not_installed("rvest")
skip_if_too_deep(5)
app_driver <- app_driver_tm_file_viewer()

Expand All @@ -42,6 +43,7 @@ test_that("e2e - tm_file_viewer: Initializes without errors and shows files tree
})

test_that("e2e - tm_file_viewer: Shows selected image file", {
testthat::skip_if_not_installed("rvest")
skip_if_too_deep(5)
app_driver <- app_driver_tm_file_viewer()

Expand All @@ -58,6 +60,7 @@ test_that("e2e - tm_file_viewer: Shows selected image file", {
})

test_that("e2e - tm_file_viewer: Shows selected text file", {
testthat::skip_if_not_installed("rvest")
skip_if_too_deep(5)
app_driver <- app_driver_tm_file_viewer()

Expand Down
1 change: 1 addition & 0 deletions tests/testthat/test-shinytest2-tm_misssing_data.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ test_that("e2e - tm_missing_data: Validate functionality and UI response for 'By
})

test_that("e2e - tm_missing_data: Validate 'By Variable Levels' table values", {
testthat::skip_if_not_installed("rvest")
skip_if_too_deep(5)
app_driver <- app_driver_tm_missing_data()

Expand Down
Loading