1040 rtables round type #366

iaugusty · 2025-11-27T14:01:48Z

this PR is linked to rtables issue insightsengineering/rtables#1040

…ing in certain situation - introduce getter and setter for round_type

iaugusty · 2025-11-27T14:05:22Z

I have read the CLA Document and I hereby sign the CLA

github-actions · 2025-11-27T14:09:19Z

Unit Tests Summary

1 files 8 suites 14s ⏱️
57 tests 57 ✅ 0 💤 0 ❌
431 runs 431 ✅ 0 💤 0 ❌

Results for commit eb17984.

♻️ This comment has been updated with latest results.

github-actions · 2025-11-27T14:09:24Z

Code Coverage Summary

Filename             Stmts    Miss  Cover    Missing
-----------------  -------  ------  -------  -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/format_value.R       272       6  97.79%   117, 233, 276, 288, 564, 572
R/generics.R           128      13  89.84%   148, 286-290, 492, 504, 535, 565, 673, 686, 707-714, 769
R/labels.R              55       7  87.27%   51, 57, 66, 107, 133, 142, 146
R/matrix_form.R        701      36  94.86%   134, 174, 410, 530-531, 623, 633-636, 654, 685, 775-776, 790-795, 825-828, 888-889, 982-983, 1038, 1090, 1256, 1292, 1349, 1397, 1400, 1404
R/mpf_exporters.R      292      31  89.38%   2-9, 110-120, 165, 201, 246, 249, 254, 435-441, 445, 448, 452, 502, 581
R/page_size.R           42       1  97.62%   219
R/pagination.R         770      56  92.73%   327-330, 435-450, 540, 595, 600, 641, 679-690, 766, 878-879, 901-910, 1051, 1054, 1275, 1312-1316, 1333-1341, 1422, 1562-1563, 1579-1580, 1594-1595
R/tostring.R           783      66  91.57%   88, 296, 351, 421, 454, 462, 498, 555-558, 594, 660-663, 669-673, 676-679, 686-691, 774-775, 915-916, 981-988, 1038-1042, 1111, 1164, 1183-1187, 1198, 1216, 1233, 1248, 1346, 1389, 1434, 1520, 1559, 1613, 1620
R/utils.R                3       0  100.00%
R/zzz.R                 17       6  64.71%   28-33
TOTAL                 3063     222  92.75%

Diff against main

Filename             Stmts    Miss  Cover
-----------------  -------  ------  -------
R/format_value.R        +7      +1  -0.32%
R/generics.R            +3      +1  -0.56%
R/matrix_form.R         +6      -1  +0.19%
R/mpf_exporters.R       +5      +3  -0.86%
TOTAL                  +21      +4  -0.08%

Results for commit: eb17984

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

shajoezhu · 2025-11-27T14:36:14Z

hi @iaugusty , can you please resolve the cicd checks. thanks

…engineering/formatters into 1040_rtables_round_type

iaugusty · 2025-11-27T16:26:18Z

@shajoezhu resolved, thx

Signed-off-by: Davide Garolini <dgarolini@gmail.com>

shajoezhu

hi @iaugusty , there is breaking changes downstream https://github.com/insightsengineering/scda.test/actions/runs/19765994667/job/56638822161?pr=201 https://github.com/insightsengineering/formatters/actions/runs/19762364980/job/56627024250 can you please take a look. Thanks!

nikolas-burkoff · 2025-11-28T14:37:08Z

Hi @shajoezhu when I run scda.test with this branch of formatters and the corresponding branches of rtables and rlistings (1040_rtables_roundtype) I don't get any errors locally.

The processes have changed since I was last involved so is there something extra we need to do?
Thanks

shajoezhu · 2025-11-28T14:51:13Z

Hi @shajoezhu when I run scda.test with this branch of formatters and the corresponding branches of rtables and rlistings (1040_rtables_roundtype) I don't get any errors locally.
The processes have changed since I was last involved so is there something extra we need to do? Thanks

Let me see how this goes https://github.com/insightsengineering/scda.test/pull/201/files

nikolas-burkoff · 2025-11-28T14:57:29Z

Yay that worked.

~~I will make similar changes in rtables and rlistings so in their PRs the DESC files reference the correct branch of formatters as well so we can make those checks pass as well.~~ Edit I see you have done this

When the code is merged into main (we still want Gabe to take a look so it won't be this week) - does the automation set insightsengineering/formatters@1040_rtables_round_type back to insightsengineering/formatters in the Desc file? Or do we have to do something else?

shajoezhu

awesome! this is working thanks! see insightsengineering/scda.test#201

gmbecker

Overall new rounding type and generics look good. Please update default rounding type in various places from a fixed value to obj_round_type of the relevant (other) argument. I think I added suggestions for all of them but do another pass to make sure I didn't miss anything.

Note this looks like a ton of issues but its really just 2, I think, but one of them happens in many places, so not nearly as bad as it looks.

After applying suggestions and other changes please test locally (and remember to roxygenize). Marking as draft so you can apply the suggestions and then pull pull the changes to do the rest an test locally. reset it to non-draft whenever you feel its ready @iaugusty

R/tostring.R

gmbecker · 2025-12-01T00:26:56Z

R/tostring.R

-                                                  round_type = c("iec", "sas")) {
+                                                  round_type = valid_round_type) {
  checkmate::assert_flag(tf_wrap)
+  round_type <- match.arg(round_type)


Suggested change

round_type <- match.arg(round_type)

Only when paired with above change

R/tostring.R

R/mpf_exporters.R

gmbecker · 2025-12-01T00:30:23Z

R/mpf_exporters.R

                          paginate = TRUE,
+                          round_type = valid_round_type,
                          ...) {
+  round_type <- match.arg(round_type)


Suggested change

round_type <- match.arg(round_type)

R/generics.R

gmbecker · 2025-12-01T00:41:20Z

R/generics.R

                                                     fontspec = font_spec(),
                                                     col_gap = mf_colgap(tt) %||% 3L,
-                                                     round_type = c("iec", "sas")) {
+                                                     round_type = valid_round_type) {


This is fine because it throws an error but could be changed to obj_round_type as with others

R/generics.R

iaugusty · 2025-12-01T16:00:29Z

updates as suggested and tested related PRs + scda.test locally. All seems fine, therefor convert back to PR

github-actions · 2025-12-01T16:30:27Z

Unit Test Performance Difference

Additional test case details

Test Suite	$Status$	Time on `main`	$±Time$	Test Case
formatters	👶		$+0.16$	Methods_for_obj_round_type

Results for commit fbe6305

♻️ This comment has been updated with latest results.

iaugusty · 2025-12-01T16:43:13Z

all checks passed, I hope we are good to go

shajoezhu

downstream all good! insightsengineering/scda.test#201 thanks guys!

iaugusty added 4 commits November 27, 2025 13:23

- add new round_type method (iec_mod) + modify behaviour of sas round…

a43e916

…ing in certain situation - introduce getter and setter for round_type

add some tests for modified rounding behaviour

701c508

document

8fa1439

update NEWS

2b3193c

iaugusty requested review from Melkiades, ayogasekaram and edelarua as code owners November 27, 2025 14:01

iaugusty requested review from gmbecker and shajoezhu November 27, 2025 14:02

[skip style] [skip vbump] Restyle files

0fbd490

iaugusty mentioned this pull request Nov 27, 2025

1040 rtables round type insightsengineering/rtables#1062

Open

iaugusty added 3 commits November 27, 2025 15:59

update roxygen to resolve failed check

edfafd9

Merge branch '1040_rtables_round_type' of https://github.com/insights…

bf57d15

…engineering/formatters into 1040_rtables_round_type

resolve roxygen + lintr problem

cfb0e78

iaugusty mentioned this pull request Nov 27, 2025

1040 rtables round type insightsengineering/rlistings#277

Open

edelarua mentioned this pull request Nov 27, 2025

[CRAN Release]: <0.5.12> #365

Open

34 tasks

Merge branch 'main' into 1040_rtables_round_type

0e5f341

Signed-off-by: Davide Garolini <dgarolini@gmail.com>

shajoezhu requested changes Nov 28, 2025

View reviewed changes

shajoezhu approved these changes Nov 28, 2025

View reviewed changes

gmbecker requested changes Dec 1, 2025

View reviewed changes

gmbecker marked this pull request as draft December 1, 2025 00:50

apply suggestions per review

d36144c

default for round_type when input can be a list

218820a

iaugusty marked this pull request as ready for review December 1, 2025 16:00

github-actions bot and others added 3 commits December 1, 2025 16:03

[skip style] [skip vbump] Restyle files

b788905

WORDLIST update

808511c

empty commit

aad4f07

resolve lintr issues

eb17984

shajoezhu requested a review from gmbecker December 2, 2025 00:51

shajoezhu approved these changes Dec 2, 2025

View reviewed changes

Uh oh!

1040 rtables round type #366

Are you sure you want to change the base?

1040 rtables round type #366

Uh oh!

Conversation

iaugusty commented Nov 27, 2025

Uh oh!

iaugusty commented Nov 27, 2025

Uh oh!

github-actions bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Tests Summary

Uh oh!

github-actions bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Diff against main

Uh oh!

shajoezhu commented Nov 27, 2025

Uh oh!

iaugusty commented Nov 27, 2025

Uh oh!

shajoezhu left a comment

Choose a reason for hiding this comment

Uh oh!

nikolas-burkoff commented Nov 28, 2025

Uh oh!

shajoezhu commented Nov 28, 2025

Uh oh!

nikolas-burkoff commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shajoezhu left a comment

Choose a reason for hiding this comment

Uh oh!

gmbecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmbecker Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

gmbecker Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gmbecker Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gmbecker Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iaugusty commented Dec 1, 2025

Uh oh!

github-actions bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Performance Difference

Uh oh!

iaugusty commented Dec 1, 2025

Uh oh!

shajoezhu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions bot commented Nov 27, 2025 •

edited

Loading

github-actions bot commented Nov 27, 2025 •

edited

Loading

nikolas-burkoff commented Nov 28, 2025 •

edited

Loading

github-actions bot commented Dec 1, 2025 •

edited

Loading