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

Pagination and column widths handling #937

Merged
merged 10 commits into from
Oct 7, 2024
Merged

Conversation

Melkiades
Copy link
Contributor

@Melkiades Melkiades commented Sep 25, 2024

Fixes #924 and hopefully #925

It seems that the pagination needs a lot of additional work to function correctly. It may be better to work on it in the next PI. @shajoezhu fyi here pagination works but column widths and lpp/cpp estimations are a bit difficult to concretize consistently

@Melkiades Melkiades added the sme label Sep 25, 2024
@shajoezhu
Copy link
Collaborator

i start thinking again that we need another downstream package for exporting

@Melkiades
Copy link
Contributor Author

i start thinking again that we need another downstream package for exporting

The export functions are in {rtables} when available only for tables and {formatters} when for both tables and listings. Making a separate package would make sense if we wanted to cover more objects, but I do not see this happening soon. Also, exporters are relatively small; the larger function here is the flextable transformer, for which there are a lot of aesthetics that are hard to test.

In summary, we already have a complex dependency tree, and we might want to keep dev exporters in {rtables} and full exporters (w/ listings capabilities) in {formatters}. Beside that I do not see too much the advantage of having exporters in a separate package atm. Also the risk of circular deps would be very real here, forcing us to move function around beyond what we need to.

I am open to discussing this anyway! Maybe a pros and cons next PI? :)

@Melkiades Melkiades marked this pull request as ready for review October 2, 2024 16:39
@Melkiades
Copy link
Contributor Author

Fixes #924 and hopefully #925

I managed to make these work, but the estimation of the actual page dimension is very faulty. It will need more design for that

Copy link
Contributor

github-actions bot commented Oct 2, 2024

Unit Tests Summary

    1 files     28 suites   1m 42s ⏱️
  218 tests   218 ✅ 0 💤 0 ❌
1 577 runs  1 577 ✅ 0 💤 0 ❌

Results for commit c1960db.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Oct 2, 2024

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
Exporting to txt, pdf, rtf, and docx 👶 $+0.28$ export_as_doc_produces_a_warning_if_manual_column_widths_are_used
tt_as_flextable 👶 $+0.55$ check_colwidths_in_flextable_object

Results for commit 74b7a0c

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Oct 4, 2024

badge

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/00tabletrees.R               780      63  91.92%   20, 94, 97, 428, 519-520, 523, 681, 785, 877-878, 980, 983, 985-986, 1004-1007, 1027, 1142-1145, 1243-1248, 1404, 1504-1507, 1573-1576, 1612-1615, 1621-1626, 1677, 1684, 1778, 1886, 1899, 1902-1905, 1908-1911, 1938, 1969-1970
R/as_html.R                    167      25  85.03%   5-10, 77, 149-154, 159-164, 179-183, 270
R/colby_constructors.R         594      26  95.62%   81, 134, 197-200, 267-270, 411, 427, 1177, 1265, 1426, 1465, 1476, 1484, 1487, 1511, 1532, 1678, 1901-1904
R/compare_rtables.R             83      17  79.52%   93-96, 99-102, 115-118, 137, 156-157, 188, 193
R/custom_split_funs.R          265      40  84.91%   127, 132, 138-143, 156, 173-177, 353-358, 375-380, 456, 502, 518-521, 537, 599, 609-610, 612, 624, 668, 693
R/default_split_funs.R         286      22  92.31%   271, 334-337, 348-349, 351, 353, 550-554, 618-621, 684-687
R/format_rcell.R                12       0  100.00%
R/indent.R                      13       2  84.62%   40-41
R/index_footnotes.R             66       0  100.00%
R/make_split_fun.R             138      31  77.54%   22-26, 36-39, 52-55, 58-61, 115, 119, 267, 270-273, 278-281, 295, 366, 375, 377, 379, 430
R/make_subset_expr.R           137      15  89.05%   35, 47-61, 135-142, 178, 267, 271, 280
R/summary.R                    144      38  73.61%   35, 80, 178-220, 269, 315-331, 366, 397
R/tree_accessors.R            1119     140  87.49%   110, 139-140, 264, 284, 310, 333, 363, 381, 400-404, 426, 448-451, 566, 593-594, 880-886, 1030, 1049, 1075, 1127, 1184-1185, 1222, 1257, 1295-1300, 1359, 1433-1437, 1455-1464, 1542, 1662-1665, 1690, 1712-1713, 1723, 1774, 1795-1800, 1821-1826, 1837, 1911, 1952, 2051, 2158, 2171, 2185, 2201, 2210, 2220-2224, 2274-2279, 2482, 2492-2495, 2505, 2530-2533, 2540, 2542-2545, 2667, 2701-2702, 2759, 3064, 3425, 3541, 3575-3600, 3691-3699, 3852, 3926-3932, 4144-4145, 4152, 4155-4158, 4162, 4212, 4273, 4298-4322
R/tt_afun_utils.R              411      32  92.21%   48, 155, 162, 171-184, 250, 258-259, 477, 485-488, 570-574, 594, 608-610
R/tt_as_df.R                   223      17  92.38%   109-113, 161-164, 216-219, 364, 370, 402, 454
R/tt_as_flextable.R            456      24  94.74%   122, 165, 183, 206-210, 328, 573-576, 585, 639, 645, 674-677, 870-874
R/tt_compare_tables.R           70       4  94.29%   51, 174, 246, 250
R/tt_compatibility.R           570      70  87.72%   19, 142-143, 186, 191, 319-320, 324-327, 333, 337, 521, 575-578, 615-617, 655, 688, 708, 728-731, 741-744, 789, 806-810, 816-819, 893, 920-923, 932, 994, 1002, 1013-1016, 1127, 1134, 1162-1176, 1207-1208
R/tt_dotabulation.R           1161      95  91.82%   54, 246, 251, 253, 301, 325, 329-332, 364-367, 390, 423-426, 454-457, 552, 690-694, 743, 747, 775-778, 788, 808-812, 819-822, 1082, 1086, 1117, 1220-1223, 1433-1441, 1705-1714, 1796-1799, 1810, 1815, 1820-1821, 1823, 1834, 1839, 1862, 1948-1967
R/tt_export.R                  121      29  76.03%   45, 155, 167-194, 316
R/tt_from_df.R                  15       0  100.00%
R/tt_paginate.R                513      37  92.79%   74, 122-131, 441, 576-579, 600-604, 749-752, 803-810, 887, 890, 908, 915, 918
R/tt_pos_and_access.R          571      42  92.64%   76, 78-80, 105, 166, 212-216, 258, 507, 509, 517, 523, 537, 547-550, 730, 733, 741-745, 750-753, 780, 833-834, 845, 1007-1008, 1076-1092, 1362, 1437
R/tt_showmethods.R             162      21  87.04%   56, 91-113, 223, 249, 258, 263, 266-270, 359-360
R/tt_sort.R                    101       5  95.05%   245-248, 256
R/tt_toString.R                433      24  94.46%   123, 345, 367, 380, 390, 396, 399, 405-415, 508, 609, 811-836
R/utils.R                       34       1  97.06%   56
R/validate_table_struct.R       84      10  88.10%   80-84, 93-94, 140, 149-150
R/Viewer.R                      61       9  85.25%   46, 50, 60-64, 84, 118
TOTAL                         8790     839  90.46%

Diff against main

Filename               Stmts    Miss  Cover
-------------------  -------  ------  -------
R/tt_as_flextable.R      +44      +1  +0.32%
R/tt_export.R            +38     +26  -20.35%
TOTAL                    +82     +27  -0.22%

Results for commit: c1960db

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

@edelarua edelarua left a comment

Choose a reason for hiding this comment

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

This looks good to go!! It can be revisited at a later date if refinement is needed :)

@edelarua edelarua merged commit f2ef09d into main Oct 7, 2024
29 checks passed
@edelarua edelarua deleted the 924_page_by_and_colwidths@main branch October 7, 2024 14:30
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable page_by pagination for docx
3 participants