-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
The documentation of check_modules_datanames()
#1395
Conversation
Code Coverage Summary
Diff against main
Results for commit: 302208d Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 25 suites 8m 39s ⏱️ Results for commit 302208d. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 0a92441 ♻️ This comment has been updated with latest results. |
check_modules_datanames()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup of the code! 👍
Just a few minor comments.
I tried to see if the recursive function would perform better as an iterative call, but they both run in the order of <1ms
with the difference being negligible and attributed to the assertions (that are done over and over again)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Messages are not equivalent in the ending "Dataset available in data: ..."
I suggested the fix and recommend adding the test in the bottom (or similar)
pkgload::load_all("teal")
#> You are using teal version 0.15.2.9078
mods <- modules(example_module(datanames = c("iris", "mtcars", "co2")))
check_modules_datanames(mods, "iris")
#> [1] "Datasets \"mtcars\", \"co2\" are missing for module \"example teal module\". Datasets available in data: \"iris\""
check_modules_datanames_html(mods, "iris") |> toString() |> rvest::read_html() |> rvest::html_text() |> stringr::str_replace_all(" ?\n ?", "")
#> [1] " Datasets mtcars and co2 are missing for tab 'example teal module'. No datasets are available in data."
Created on 2024-10-28 with reprex v2.1.1
testthat::test_that("check_modules_datanames result is equivalent in text and html format", {
testthat::skip_if_not_installed("xml2")
testthat::skip_if_not_installed("rvest")
mods <- modules(
example_module(datanames = c("iris", "mtcars", "co2"))
)
text_message <- check_modules_datanames(mods, "iris")
html_message <- check_modules_datanames_html(mods, "iris")
# Cleanup messages (making text_message the template/target)
# remove quotes and backticks
text_clean <- gsub("[\"'`]", "", text_message)
html_clean <- gsub("[\"'`]", "", html_clean)
# Cleanup extra spaces
html_clean <- paste(
collapse = " ",
trimws(
strsplit(
rvest::html_text(rvest::minimal_html(html_message), trim = TRUE),
"\n"
)[[1]]
)
)
# Replace " and " to ", " to be similar to text
html_clean <- gsub(" and ", ", ", html_clean, fixed = TRUE)
# Convert from UI's "tab" to warning's "module" to be similar to text warning
html_clean <- gsub("are missing for tab", "are missing for module", html_clean, fixed = TRUE)
# Replace singular to plural
html_clean <- gsub("Dataset ", "Datasets ", html_clean)
# Remove extra full stop in html
html_clean <- gsub("[.]$", "", html_clean)
# String should now be the same
testthat::expect_identical(html_clean, text_clean)
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
You made this manual conversion beautiful 💯
I left one **optional** (esthetics) suggestion that removes extra space before the commas.
Closes #1321
Little refurnishment:
check_modules_datanames
to two functions, one to returncharacter
and other to returnshiny.tag.list