Skip to content

implement optional 'sas' rounding, pass round_type and na_str to fmt fun#337

Merged
Melkiades merged 30 commits intomainfrom
336_nearest_value_round
Feb 28, 2025
Merged

implement optional 'sas' rounding, pass round_type and na_str to fmt fun#337
Melkiades merged 30 commits intomainfrom
336_nearest_value_round

Conversation

@gmbecker
Copy link
Collaborator

addresses #336 and #319

@gmbecker gmbecker marked this pull request as draft January 31, 2025 22:44
@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2025

Unit Tests Summary

  1 files    8 suites   15s ⏱️
 55 tests  55 ✅ 0 💤 0 ❌
416 runs  416 ✅ 0 💤 0 ❌

Results for commit c08fdfb.

♻️ This comment has been updated with latest results.

@gmbecker gmbecker marked this pull request as ready for review February 3, 2025 21:30
@gmbecker
Copy link
Collaborator Author

gmbecker commented Feb 4, 2025

@cicdguy looks like the commit pushed by the restyle files job is failing the style check

@cicdguy
Copy link
Contributor

cicdguy commented Feb 4, 2025

@cicdguy looks like the commit pushed by the restyle files job is failing the style check

This is actually valid behavior, where the commit has the [skip style] message embedded in it and it doesn't recursively attempt to restyle the files, so it cancels the workflow. Unfortunately GitHub doesn't allow a "true skip" option via custom commit messages or even allow a "true skip" API for the GH Actions workflows, so we have to cancel the workflow while running the workflow which shows up as a red cross.

This is the relevant line that tells you that the workflow has been canceled: https://github.com/insightsengineering/formatters/actions/runs/13125255731/job/36623112402?pr=337#step:7:14

Skip instruction detected - cancelling the workflow.

@gmbecker
Copy link
Collaborator Author

gmbecker commented Feb 4, 2025

Hmm, it'd be nice if we could get rid of the x. the PR is still showing as "not passing". @shajoezhu @Melkiades sounds like this PR is "effectively passed" and ready for review.

@shajoezhu shajoezhu added the sme Tracks changes for the sme board label Feb 7, 2025
@shajoezhu
Copy link
Contributor

Hmm, it'd be nice if we could get rid of the x. the PR is still showing as "not passing". @shajoezhu @Melkiades sounds like this PR is "effectively passed" and ready for review.

you could do a "empty commit" to trigger the pipeline after the CI restyle btw.

@shajoezhu
Copy link
Contributor

Hi @gmbecker , I would suggest in insightsengineering/scda.test#183, if you add a snapshot test, I know that jnj don't have this ready yet, so I think it would be nice if you could add a snapshot test into scda.test, so in the future, we are aware that new changes will cause breaking change unnoticed.

@Melkiades
Copy link
Contributor

@gmbecker could you please update the news file?

new PR for #333

---------

Signed-off-by: Joe Zhu <sha.joe.zhu@gmail.com>
Co-authored-by: Kamil <tazzpl@gmail.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
@shajoezhu shajoezhu self-assigned this Feb 24, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Feb 26, 2025

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
formatters 👶 $+0.12$ sas_style_rounding_works

Results for commit be4902c

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 26, 2025

badge

Code Coverage Summary

Filename             Stmts    Miss  Cover    Missing
-----------------  -------  ------  -------  -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/format_value.R       262       5  98.09%   110, 216, 255, 523, 531
R/generics.R           125      12  90.40%   148, 286-290, 492, 504, 535, 565, 673, 686, 707-714
R/labels.R              55       7  87.27%   51, 57, 66, 107, 133, 142, 146
R/matrix_form.R        692      41  94.08%   132, 393, 513-514, 606, 616-619, 637, 668, 758-759, 773-778, 808-811, 871-872, 961-962, 989, 1017, 1069, 1231, 1267, 1270-1274, 1324, 1372, 1375, 1379
R/mpf_exporters.R      287      28  90.24%   2, 102-112, 157, 193, 238, 241, 246, 426-432, 436, 439, 443, 491, 570
R/page_size.R           42       1  97.62%   219
R/pagination.R         764      52  93.19%   327-330, 435-450, 540, 595, 600, 641, 679-690, 766, 878-879, 901-910, 1049, 1100-1101, 1254, 1291-1295, 1312, 1319, 1402, 1542-1543, 1559-1560, 1574-1575
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                 3030     218  92.81%

Diff against main

Filename             Stmts    Miss  Cover
-----------------  -------  ------  -------
R/format_value.R       +65      -7  +4.18%
R/mpf_exporters.R      +10      +8  -2.54%
R/pagination.R          +5       0  +0.04%
TOTAL                  +80      +1  +0.16%

Results for commit: c08fdfb

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

@Melkiades Melkiades left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! Thanks Gabe. I will approve the other PRs. Please fix lintr (i think it is return() things, dont know why it is firing now)

@Melkiades Melkiades merged commit bc14bb9 into main Feb 28, 2025
29 checks passed
@Melkiades Melkiades deleted the 336_nearest_value_round branch February 28, 2025 08:43
@github-actions github-actions bot locked and limited conversation to collaborators Feb 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

sme Tracks changes for the sme board

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants