1040 rtables round type #1062
1040 rtables round type #1062
Conversation
- update round_type = c("iec", "sas") to round_type = valid_round_type (introduced in formatters)
iaugusty
requested review from
Melkiades,
ayogasekaram and
edelarua
as code owners
|
✅ All contributors have signed the CLA |
|
I have read the CLA Document and I hereby sign the CLA |
| max_width = max_width, | ||
| fontspec = fontspec, | ||
| ttype_ok = ttype_ok, | ||
| round_type = round_type |
Signed-off-by: Joe Zhu <joe.zhu@roche.com>
Unit Tests Summary 1 files 29 suites 1m 53s ⏱️ Results for commit 44970a0. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit a9fa6c8 ♻️ This comment has been updated with latest results. |
|
hi @iaugusty @nikolas-burkoff , can you take a look at this |
|
@shajoezhu I've pushed a fix for the broken test. It was from the new "default" format that has been added in the interim by the separate pr on formatters, nothing wrong with the code changes here. @iaugusty please fix the documentation notes then roxygenize, and make sure the pkgdown builds ok (currently missing at least one topic, edit |
|
Code Coverage SummaryDiff against mainResults for commit: 44970a0 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
|
hi @gmbecker , yes, i tested downstream in scda.test and ran it as a whole cohort for formatters, rtables and rlistings, all working. thanks a lot for the changes can you please update the news, we can merge these now. no blockers from my side. thank you |
|
@shajoezhu @gmbecker can I be added as a collaborator to insights engineering? Thanks |
|
all checks passed, I hope we are good to go |
shajoezhu
left a comment
shajoezhu
left a comment
There was a problem hiding this comment.
downstream all good! insightsengineering/scda.test#201 thanks guys!
please update description after formatters pr is merged in
hi @nikolas-burkoff , there is an invite on your way, you will have access to formatters/rlistings/rtables/tern/scda.test, let me know if problem. thanks! |
|
Thanks @shajoezhu - the invite is asking me to authenticate with a single sign on with Roche credentials (which I don't have). |
hi @nikolas-burkoff , can we set a call and you can show me what you see, my email is joe.zhu@roche.com, thanks! |
gmbecker
left a comment
gmbecker
left a comment
There was a problem hiding this comment.
Overall looks good a few more tests to add
tests/testthat/test-round_type.R
Outdated
| }) | ||
|
|
||
|
|
||
| test_that("toString method works correctly with user defined round_type", { |
There was a problem hiding this comment.
reword this, user is setting round_type but not defining a new one
tests/testthat/test-round_type.R
Outdated
| txtvals_iec <- vals_round_type_fmt(vals = vals_round_type, round_type = "iec") | ||
| txtvals_sas <- vals_round_type_fmt(vals = vals_round_type, round_type = "sas") | ||
|
|
||
| # confirm that at least one value is different |
There was a problem hiding this comment.
update comment to be more explicit about expected behavior (first two different third same)
| df_iec2 | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Add testing for explicit calls to export_as_txt
tests/testthat/test-round_type.R
Outdated
| mpf <- matrix_form(tbl_sas) | ||
|
|
||
| expect_identical( | ||
| mpf$round_type, |
There was a problem hiding this comment.
use the accessor here and elsewhere
tests/testthat/test-round_type.R
Outdated
| }) | ||
|
|
||
| test_that("test for obj_round_type setter", { | ||
| skip_if_not_installed("dplyr") |
There was a problem hiding this comment.
I dont' think we use dplyr here?
tests/testthat/test-round_type.R
Outdated
| rep("iec", 3) | ||
| ) | ||
|
|
||
| gkids <- tree_children(kids[[1]]) |
There was a problem hiding this comment.
make a function which checks the children then we can call it here and below
| exp_str_sas | ||
| ) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Add tests for overriding round type in toString/export_as_txt
toString(..., round_type = "iec_mod")
Add test for matrix_form method (make sure it ends up with the right round_type)
|
@gmbecker All your comments have been addressed in updated tests. |
this PR is linked to
formatters PR insightsengineering/formatters#366
rlistings PR insightsengineering/rlistings#277