-
-
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
Improvement on decorators #822
Conversation
Unit Tests Summary 1 files 22 suites 13m 32s ⏱️ Results for commit 0b98cf4. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 74972fd ♻️ This comment has been updated with latest results. |
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.
tm_data_table: decorators removed
Sorry, I don't recall why we remove decorators only on this module. Could you remind me? Is it because it cannot be used with a reporter?
tm_missing_data: Decorators using rlisting instead of DataTable
When checking this module on ADRS tab "By Variable Levels" I get this error on the decorator (New row):
Error when executing the `data` module: is(tbl, "VTableTree") is not TRUE
when evaluating qenv code:
table <- rtables::insert_rrow(table, rtables::rrow("table row"))
Check your inputs or contact app developer if error persists.
The decorator works for the tm_t_crosstable
module so I guess the problem is on tm_missing_data
.
tm_distribution: Was decorating data.frames in report, moved to rlisting
I don't see the decorator on the summary or test table but I do on the reporter (could be a problem on my machine):
tm_outliers: Code improvement
Looks good, minor suggestion: on line 892 there are multiple calls to commong_code_q()
I would save one as qenv and reuse the qevn for extracting other dataset. Sorry it doesn't let me make the suggestion to code that wasn't modified.
Co-authored-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com> Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Exactly! Only objects that appear on the reporter are decorated. This also connects with the screenshot below
Thanks, there was this last leftover wrong decorator in the app it should use
The decorator feature scope is limited to
I suppose you mean this chunk? |
Yes! I meant that chunk highlighted on the image! Feel free to ping me when the PR is ready for another pass (did you forget to push?). |
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.
Looks good! Great work!
Pull Request
Fixes https://github.com/insightsengineering/coredev-tasks/issues/605
Changes description
utils.R
functionsRevert ggplot2_args to roxygen2@template
roxygen2
documentation.{lifecycle}
dependencyModules that need recheck (for reviewer):
tm_data_table
: decorators removedtm_missing_data
: Decorators using rlisting instead of DataTabletm_distribution
: Was decorating data.frames in report, moved to rlistingtm_outliers
: Code improvementBig example app