-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add ability to set datanames
#824
base: main
Are you sure you want to change the base?
Conversation
Unit Tests Summary 1 files 22 suites 12m 58s ⏱️ Results for commit 4f8e966. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 88bde23 ♻️ This comment has been updated with latest results. |
Merge branch '821_select_datanames@main' of https://github.com/insightsengineering/teal.modules.general into 821_select_datanames@main # Conflicts: # man/tm_front_page.Rd
Signed-off-by: Lluís Revilla <[email protected]>
…tsengineering/teal.modules.general into 821_select_datanames@main
Co-authored-by: Dawid Kałędkowski <[email protected]> Signed-off-by: Lluís Revilla <[email protected]>
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.
Thanks, I'll fix the style checks for the documentation too.
…tsengineering/teal.modules.general into 821_select_datanames@main
… of srv_page_data_table
… datanames argument.
Co-authored-by: Dawid Kałędkowski <[email protected]> Signed-off-by: Lluís Revilla <[email protected]>
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.
Please remove the tests for deprecated messages. Warning is thrown by the tm_
constructor. Testing it through the app driver doesn't make any sense as the message is not thrown in shiny runtime.
When I'm re-running the cases above, I'm still facing an issue with the
I'm exploring why this is different from other modules |
You've stepped into funny bug. |
I approved the PR mentioned. If there is anything else to improve let me know |
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.
Thanks 👍
I can reproduce the test failing locally now (before they were sporadic/ not consistent) running through all this branch's commits to tests which one broke them ( |
Bisect finished:
But results don't seem deterministic, as the commit can be both good and bad to run the tests. |
I can now reproduce the failing checks locally (both on Rstudio R terminal and on the check window pane). Running the tests manually, shows additional information. From this failing test, whose error is reported as:
I run the code manually and see more details, the app Detailsload_all("../teal.modules.general")
data <- within(simple_teal_data(), {
set.seed(123)
add_nas <- function(x) {
x[sample(seq_along(x), floor(length(x) * runif(1, .05, .17)))] <- NA
x
}
iris[] <- lapply(iris, add_nas)
mtcars[] <- lapply(mtcars, add_nas)
mtcars[["cyl"]] <- as.factor(mtcars[["cyl"]])
mtcars[["gear"]] <- as.factor(mtcars[["gear"]])
})
app <- teal::init(
data = data,
modules = tm_missing_data(
label = "Missing data",
plot_height = c(600, 400, 5000),
plot_width = NULL,
parent_dataname = "",
ggtheme = "gray",
ggplot2_args = list(
"Combinations Hist" = teal.widgets::ggplot2_args(
labs = list(subtitle = "Plot produced by Missing Data Module", caption = NULL)
),
"Combinations Main" = teal.widgets::ggplot2_args(labs = list(title = NULL))
),
pre_output = NULL,
post_output = NULL
)
)
Created on 2025-01-16 with reprex v2.1.1 I fixed those and pushed to see if anything else come up. |
I thought that the remaining failing tests could be due to the code generated by each branch. But running them and comparing the (html) code generated, didn't help to identify the issue.
It might be either some hidden component, but I'm not familiar with this part of shiny/testing/web development |
…tsengineering/teal.modules.general into 821_select_datanames@main
@gogonzo All checks pass now. If there is no more comments I'll merge the PR this afternoon. Thanks for helping with the ordering issue. |
Pull Request
Fixes #821
This PR add a parameter (or uses existing ones) to use on
teal::module(datanames)
to select which datasets are shown in each module.tm_file_viewer()
doesn't have and I think shouldn't have.Acceptance criteria:
tm_missing_data example
tm_variable_browser example
tm_front_page (additional modifications to enable granular selection): Example
I checked via searching
datanames =
that there are now at least 15 cases inmodule()
corresponding to the 15 modules exported.There is one that is not configurable (
datanames = NULL
on the picture above),tm_file_viewer
. In my opinion it doesn't matter the datasets available on the module to visualize files. But if needed we can add adataset_selected
argument.Note that I added the
dataset_selected
argument to two modules on the best place I thought, if a package/user is using positional arguments it might break (and also teal.gallery).Note that it doesn't show on shinylive (on Rstudio, preview, not sure if once fully installed) and also that it doesn't show the url (this could be an issue with my machine):