-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
1441 Introduce transformators parameter in modules #1313
Conversation
Unit Tests Summary 1 files 70 suites 1h 10m 52s ⏱️ Results for commit e59bc95. ♻️ 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.
36 R files modified, only remaining ones are these:
$ ls -1 | grep -v "^tm_"
argument_convention.R
arm_ref_comp.R
data.R
facet_grid_formula.R
labels.R
string_ops.R
substitute_names.R
teal.modules.clinical.R
utils.R
validate_standard_inputs.R
zzz.R
Which don't have teal modules. Nice diff!
I only see a note that could affect the release:
❯ checking installed package size ... NOTE
installed size is 5.4Mb
sub-directories of 1Mb or more:
R 1.3Mb
doc 2.7Mb
But I don't think we can do anything else...
Unit Test Performance DifferenceTest suite performance difference
Additional test case details
Results for commit d931c91 ♻️ This comment has been updated with latest results. |
Hi @llrs-roche would you mind re-reviewing again? I added changes to unify decorators parameter to be |
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.
Changes look good and the example app worked well. I didn't fully check as it might take too long ...
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.
One minor note before merging to really check for problems with decorators.
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.
LGTM: there is a superLinter warning about using only double-quotes
insightsengineering/teal#1441
Included
transformators
parameter in modules. This is passed toteal::module
that does assertions and checks on this parameter.A food for thought: default
decorators
value isNULL
where defaulttransformators
value islist()
. Maybe we can introduce a check, that allowstransformators
to beNULL
on default, and which changesNULL
tolist()
.