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

add page number #227

Merged
merged 21 commits into from
Mar 1, 2024
Merged

add page number #227

merged 21 commits into from
Mar 1, 2024

Conversation

clarkliming
Copy link
Contributor

@clarkliming clarkliming commented Nov 21, 2023

add page number in tables fixes #160

Copy link
Contributor

github-actions bot commented Nov 21, 2023

badge

Code Coverage Summary

Filename             Stmts    Miss  Cover    Missing
-----------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------------------
R/format_value.R       194      12  93.81%   88, 104-111, 189, 301, 401, 412, 420
R/generics.R           105       7  93.33%   454, 466, 499, 528, 652-658
R/labels.R              55       7  87.27%   49, 55, 64, 105, 133, 142, 146
R/matrix_form.R        552      50  90.94%   103, 455-456, 544, 557-560, 579, 611, 705-706, 721-726, 756-759, 792-793, 825-826, 870, 1033, 1057-1079, 1121, 1172, 1175, 1179
R/mpf_exporters.R      238      19  92.02%   2, 90-92, 201, 241, 246, 429, 432, 438-441, 479, 551-554, 567
R/page_size.R           45       1  97.78%   172
R/pagination.R         634      49  92.27%   250, 302-305, 407-420, 582, 765-766, 787-797, 1000-1016, 1148, 1155, 1362-1363, 1375-1376, 1390-1391
R/tostring.R           586      49  91.64%   41, 93, 162, 196, 204, 240, 297-300, 393-397, 400-403, 410-415, 492, 631-632, 697-704, 761-765, 849, 864, 958, 1010, 1051, 1096, 1151, 1158
R/utils.R                3       0  100.00%
R/zzz.R                 17       6  64.71%   29-34
TOTAL                 2429     200  91.77%

Diff against main

Filename             Stmts    Miss  Cover
-----------------  -------  ------  -------
R/mpf_exporters.R       +2       0  +0.07%
R/pagination.R         +24       0  +0.30%
R/tostring.R           -14      -6  +0.80%
R/zzz.R                +17      +6  +64.71%
TOTAL                  +29       0  +0.10%

Results for commit: 7eed056

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Nov 21, 2023

Unit Tests Summary

  1 files    5 suites   10s ⏱️
 42 tests  42 ✅ 0 💤 0 ❌
294 runs  294 ✅ 0 💤 0 ❌

Results for commit 7eed056.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Nov 21, 2023

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
pagination 💔 $3.79$ $+1.70$ $+7$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
pagination 👶 $+1.46$ pag_num_works_in_paginate_to_mpfs_and_export_as_txt

Results for commit d4edf6c

♻️ This comment has been updated with latest results.

@edelarua edelarua added the sme Tracks changes for the sme board label Nov 22, 2023
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.

Thanks Liming for working on the page numbering. We need to have a more general solution here in my opinion. Also, I would like some consensus on how this page numbering should appear before committing to it. What do you think @edelarua @ayogasekaram @shajoezhu?

@ayogasekaram
Copy link
Contributor

Thanks Liming for working on the page numbering. We need to have a more general solution here in my opinion. Also, I would like some consensus on how this page numbering should appear before committing to it. What do you think @edelarua @ayogasekaram @shajoezhu?

I think page x/n is clear so I am comfortable with this solution. I think anything longer or wordier would interfere with the table aesthetics.

@edelarua edelarua linked an issue Nov 27, 2023 that may be closed by this pull request
@shajoezhu shajoezhu added the poc label Nov 28, 2023
@Melkiades Melkiades self-assigned this Feb 27, 2024
R/pagination.R Outdated Show resolved Hide resolved
@Melkiades
Copy link
Contributor

fyi this should be ready @shajoezhu @BFalquet

@shajoezhu shajoezhu requested a review from Melkiades March 1, 2024 04:18
Copy link
Contributor

@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.

approve this to unblock merging

R/mpf_exporters.R Outdated Show resolved Hide resolved
R/pagination.R Outdated Show resolved Hide resolved
R/pagination.R Outdated Show resolved Hide resolved
@BFalquet
Copy link

BFalquet commented Mar 1, 2024

It looks good to me, in the downstream packages we would probably have to think a bit how the user will control that but I guess that a first approximation would be to adapt the export function to fetch the value of an option as you suggested. We have already adopted this strategy for the representation of NA values and we are moving toward that for other parameters so this seems coherent.

@Melkiades Melkiades merged commit c954d90 into main Mar 1, 2024
24 checks passed
@Melkiades Melkiades deleted the add_page_number branch March 1, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
poc sme Tracks changes for the sme board
Projects
None yet
Development

Successfully merging this pull request may close these issues.

page number implementation
6 participants