-
-
Notifications
You must be signed in to change notification settings - Fork 15
Generate plots inside qenv for the reporter #941
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
Conversation
…ering/teal.modules.general into 936_variable_browser@main
…ering/teal.modules.general into 936_variable_browser@main
Code Coverage SummaryDiff against mainResults for commit: 4c8ff97 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 23 suites 15m 44s ⏱️ For more details on these failures, see this check. Results for commit 4c8ff97. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 38b63dd ♻️ This comment has been updated with latest results. |
…ering/teal.modules.general into 936_variable_browser@main
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.
Pull Request Overview
This PR refactors the plot_var_summary function to work with teal_data objects (qenv) instead of raw vectors, enabling better code tracking and reproducibility. The changes support integration with the teal.reporter framework for generating reproducible reports.
- Refactored
plot_var_summaryto acceptqenv(teal_data object) instead of rawvarandvar_labparameters - Added
filter_outliersfunction to return logical vectors suitable fordplyr::filter() - Modified internal code to build plots within
teal_dataenvironment usingwithin()calls
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| man/plot_var_summary.Rd | Updated function signature documentation to reflect new qenv parameter instead of var and var_lab |
| man/filter_outliers.Rd | Added documentation for new filter_outliers helper function |
| R/tm_variable_browser.R | Refactored plot_var_summary to work with qenv, added filter_outliers, updated callers to use new API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Tested with #913
And I was able to add a card and see the output with the code.
Great job @llrs-roche
Pull Request
Fixes #936
Generates code inside the qenv for it to be reproducible and make the module functional.
I tried to make minimal changes but at the same time to make it fast to render by adding new
req()to invalidate reactives.Note that the code created for the plot has some changes as it has the construct for the new S7 labels. Maybe there are other issues with it.