Fixes for exporter pagination migration

[Melkiades]

Fixes #627

@gmbecker I updated your PR and posted it ;) Thanks for the many fixes! It is also evident that rstudio does not help much with its invisible spaces ahah

One thing: we need to talk about brackets! I found maybe a solution to the devtools AND check warnings caused by the base [ and [<- reexports (apparently roxygen creates automatically reexport aliases when you directly export another namespace function, thus creating two .rd with reexports aliases but in different files, hence warning in checks (see comments on code)). I went on reading how tibble does the same and it seems that uses S3 generics instead but w/o any reexports of base functions. So I tried to just remove the reexports and every test passes fine as brackets are dependent on the S4 generics as you designed them.

I still remember though that we already mentioned this, so we need to discuss this solution (weirdly it works btw, very unexpectedly).

[github-actions]

Code Coverage Summary

Filename                  Stmts    Miss  Cover    Missing
----------------------  -------  ------  -------  ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/00tabletrees.R            631      45  92.87%   74, 76, 345, 420-421, 424, 561, 652, 733-734, 824, 826-827, 849-850, 870, 1047-1050, 1180, 1264-1265, 1323-1326, 1359-1360, 1366-1371, 1414, 1420, 1509, 1601, 1612, 1614-1615, 1618-1619, 1647, 1672-1673
R/as_html.R                  88       7  92.05%   7-12, 68
R/colby_constructors.R      490      17  96.53%   63, 106-107, 155-156, 202-203, 328, 339, 1061, 1136, 1279, 1315, 1334, 1354, 1371, 1515
R/compare_rtables.R          78      11  85.90%   95-96, 99-100, 113-114, 131, 150-151, 181, 185
R/format_rcell.R             12       0  100.00%
R/indent.R                   13       2  84.62%   39-40
R/index_footnotes.R          52       0  100.00%
R/make_split_fun.R           97      15  84.54%   22-25, 48-49, 51-52, 103, 106, 252, 254-255, 259-260, 276, 366
R/make_subset_expr.R        106      12  88.68%   26-35, 93-98, 131, 197, 208, 215
R/simple_analysis.R           5       1  80.00%   48
R/split_funs.R              421      54  87.17%   131, 135, 140-145, 149, 159-161, 195-196, 318-321, 337-342, 412, 452, 466-467, 481, 537, 546-547, 549, 561, 601, 621, 775, 782, 807-808, 816-817, 819-820, 978-979, 990-993, 1056-1057, 1113-1114
R/summary.R                 187      20  89.30%   40, 84, 188, 195, 260-263, 273-274, 293-294, 399, 453-457, 485, 513
R/tree_accessors.R          798      74  90.73%   93, 210, 223, 240, 271, 281, 293, 385, 407-408, 662-666, 774, 788, 805, 846, 900-901, 933, 959, 991-995, 1042, 1104-1106, 1120, 1186, 1272-1273, 1291, 1305-1306, 1314, 1344, 1357-1360, 1378-1381, 1445, 1483, 1573, 1651, 1661, 1671, 1680, 1686, 1692-1695, 1973, 2250, 2344, 2426-2432, 2576, 2675, 2698-2726, 2738-2743
R/tt_afun_utils.R           345      25  92.75%   44, 146, 152, 160-169, 218, 229-230, 452, 459-460, 524-526, 545, 559-560
R/tt_compare_tables.R        65       4  93.85%   56, 170, 248, 251
R/tt_compatibility.R        413      50  87.89%   19, 137-138, 187, 191, 321-322, 325-328, 335, 338, 365, 371-372, 442, 484, 513, 531, 555-556, 596, 609-611, 680, 705-708, 717, 769, 775, 782-783, 887, 893, 919-931, 960-961
R/tt_dotabulation.R         740      44  94.05%   36, 202, 204, 247, 268, 271-272, 318, 350-351, 372, 379-380, 460, 582-584, 633, 636, 665, 852, 855, 882, 993-994, 1110-1114, 1149-1153, 1250, 1343-1351
R/tt_export.R               159      40  74.84%   47, 63, 105-118, 270-271, 277, 320-326, 330-332, 428-429, 441, 509-518
R/tt_from_df.R                9       0  100.00%
R/tt_paginate.R             381      26  93.18%   47, 69, 100-105, 355, 456-457, 474-476, 619-620, 667-672, 734, 736, 745, 751, 754
R/tt_pos_and_access.R       519      37  92.87%   73, 75-76, 100, 152, 187-189, 234, 471, 473, 480, 486, 499, 508-509, 673, 684-686, 692-695, 723, 765-766, 776, 927-928, 976-1000, 1224, 1294
R/tt_showmethods.R          121      20  83.47%   47, 73-87, 138, 154-157, 163, 170, 172-174, 252-253
R/tt_sort.R                  76       3  96.05%   211-212, 218
R/tt_toString.R             325      27  91.69%   112, 305, 318, 327, 334, 336, 342-352, 435, 489, 495, 534-537, 704-729
R/utils.R                    15       1  93.33%   109
R/Viewer.R                   56       9  83.93%   51, 55, 65-68, 87-88, 118
TOTAL                      6202     544  91.23%

Diff against main

Filename                  Stmts    Miss  Cover
----------------------  -------  ------  --------
R/00tabletrees.R             +7      +4  -0.56%
R/colby_constructors.R       +8       0  +0.06%
R/index_footnotes.R          +2       0  +100.00%
R/make_split_fun.R          +97     +15  +84.54%
R/split_funs.R                0      -1  +0.24%
R/summary.R                  +4      +4  -1.95%
R/tree_accessors.R           +5      +6  -0.70%
R/tt_dotabulation.R          +3       0  +0.02%
R/tt_export.R               -70     -33  +6.72%
R/tt_paginate.R              -2     +11  -2.91%
R/tt_pos_and_access.R        -1       0  -0.01%
R/tt_showmethods.R            0      -1  +0.83%
R/tt_sort.R                  -5      -3  +3.46%
R/tt_toString.R             +20      +5  -1.09%
R/utils.R                    +4       0  +2.42%
TOTAL                       +72      +7  -0.01%

Results for commit: 6af4174

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

    1 files    19 suites   1m 10s ⏱️
166 tests 166 ✔️ 0 💤 0 ❌
680 runs  680 ✔️ 0 💤 0 ❌

Results for commit a533588.

♻️ This comment has been updated with latest results.

[Melkiades]

I tried many combinations of inputs and roxygen2 calls. Then, I checked how they do in tibble here in sub.R, and it seems that their way is to have only one function per overloading operator (they use S3 generics as base R) and I thought to do the same here.

tibble:

Before rtables:

Now rtables:

Information about the input values can be dispatched through the documentation. Here is the old reference page on main:

Here is the new one from rstudio on this branch:

The main change in practice is that S4 methods are no more responsible for dispatching the call to different functions, depending on the input indices (e.g. if they are logical, character, or missing) but they are directly dispatched inside the function.

Let me know @gmbecker what you think about this solution. I personally think that makes also the reference page clearer and more compact.

[gmbecker]

I tried many combinations of inputs and roxygen2 calls. Then, I checked how they do in tibble here in sub.R, and it seems that their way is to have only one function per overloading operator (they use S3 generics as base R) and I thought to do the same here.

tibble:

Before rtables:

Now rtables:

Information about the input values can be dispatched through the documentation. Here is the old reference page on main:

Here is the new one from rstudio on this branch:

The main change in practice is that S4 methods are no more responsible for dispatching the call to different functions, depending on the input indices (e.g. if they are logical, character, or missing) but they are directly dispatched inside the function.

Let me know @gmbecker what you think about this solution. I personally think that makes also the reference page clearer and more compact.

I don't like this solution because it actively bypasses method dispatch in order to then reimplement it itself via duplicated code.

It is possible there is some reworking of the methods that is possible, but this late in a critical release cycle is also not the time to be touching something as fundamental as subsetting if we don't need to (and we don't need to).

Please revert this, and just document most methods as internal, so they don't show up, and a single method as non-internal.

@Melkiades

[shajoezhu]

hi @gmbecker , can you take a look at this issue please #634

[shajoezhu]

brilliant! Thanks @gmbecker ! this is good to go?

[gmbecker]

its not ready to cut the tag quite yet but yeah, its ready to merge into main in the leadup to that @shajoezhu