Skip to content
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

Fix split list for pagination when with ordered factors #235

Merged
merged 5 commits into from
Nov 26, 2024

Conversation

Melkiades
Copy link
Contributor

@Melkiades Melkiades commented Nov 22, 2024

@Melkiades Melkiades added bug Something isn't working sme labels Nov 22, 2024
Copy link
Contributor

github-actions bot commented Nov 22, 2024

Unit Tests Summary

  1 files   5 suites   9s ⏱️
 41 tests 31 ✅ 10 💤 0 ❌
111 runs  96 ✅ 15 💤 0 ❌

Results for commit 062dc53.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Nov 22, 2024

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
matrix_form 👶 $+0.06$ matrix_form_detects_or_in_labels_and_sends_meaningful_error_message

Results for commit 1536a28

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@shajoezhu shajoezhu left a comment

Choose a reason for hiding this comment

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

hi @Melkiades , thanks for the fix, tests currently failing, can you also add a test to
https://github.com/insightsengineering/scda.test/blob/main/tests/testthat/test-pagination_listing.R as well. thanks.

@Melkiades
Copy link
Contributor Author

hi @Melkiades , thanks for the fix, tests currently failing, can you also add a test to https://github.com/insightsengineering/scda.test/blob/main/tests/testthat/test-pagination_listing.R as well. thanks.

After some thinking, I do not think it is necessary to add a test there as the issue is already tested here. It is not related to pagination and it is nicely self-contained here ;)

@Melkiades Melkiades enabled auto-merge (squash) November 26, 2024 15:33
Copy link
Collaborator

@shajoezhu shajoezhu 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 @Melkiades

Copy link
Contributor

badge

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  ----------------------------------------
R/paginate_listing.R        29       0  100.00%
R/rlistings_methods.R      117      26  77.78%   18-29, 49, 65, 69, 155-158, 161, 245-251
R/rlistings.R              194      14  92.78%   180, 396-399, 403-406, 435-438, 498
TOTAL                      340      40  88.24%

Diff against main

Filename         Stmts    Miss  Cover
-------------  -------  ------  -------
R/rlistings.R      +11      +1  -0.11%
TOTAL              +11      +1  +0.09%

Results for commit: 062dc53

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@Melkiades Melkiades merged commit d74b616 into main Nov 26, 2024
28 checks passed
@Melkiades Melkiades deleted the 234_fix_split_list_ordered_factors@main branch November 26, 2024 16:02
@github-actions github-actions bot locked and limited conversation to collaborators Nov 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working request changes sme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

split_into_pages_by_var() ignores factor levels "{" character breaks the printing of listings
2 participants