Skip to content

Add guided advanced vignettes#1091

Closed
gmbecker wants to merge 9 commits into
mainfrom
add_guided_advanced
Closed

Add guided advanced vignettes#1091
gmbecker wants to merge 9 commits into
mainfrom
add_guided_advanced

Conversation

@gmbecker

@gmbecker gmbecker commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

and needed fixes/utilities for them

gmbecker and others added 2 commits June 3, 2026 15:46
Comment thread DESCRIPTION Outdated
'tt_from_df.R'
'validate_table_struct.R'
'zzz_constants.R'
Config/roxygen2/version: 8.0.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Repeated entry

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thats weird, as it wasn't manually added, but I'll look into this

Comment thread DESCRIPTION Outdated
Comment thread R/tree_accessors.R Outdated
Comment on lines +1224 to +1226
#' @rdname int_methods
#' @export
setMethod("obj_na_str", "VTableNodeInfo", function(obj) attr(obj, "row_na_strs", exact = TRUE))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Duplicated with the above. I guess it should be:
setMethod("obj_na_str", "RowsVerticalSection", function(obj) attr(obj, "row_na_strs", exact = TRUE))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

correct nice catch. thanks

Comment thread R/tt_pos_and_access.R Outdated
#' @exportMethod cell_values
setMethod(
"cell_values", "RowsVerticalSection",
function(tt, rowpath, colpath = NULL, omit_labrows = TRUE) {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This would return NULL. why should it? Could you add a comment and maybe a more explicit NULL value?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is missing code not intended to return NULL so I'll either remove it (if its not needed) or fix it.

Nice catch thanks.

Comment thread vignettes/guided_advanced_afuns_rowsverticalsection.Rmd Outdated
Comment thread R/00tabletrees.R Outdated
Comment thread R/00tabletrees.R
#' @export
c.RowsVerticalSection <- function(...) {
lst <- list(...)
if (!all(sapply(lst, function(x) inherits(x, "RowsVerticalSection")))) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would use vapply(lst, inherits, logical(1), "RowsVerticalSection") as it is a expected logical

Comment thread inst/WORDLIST
ARD
ard
ARDs
args

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

68 new words is a lot. Some look like they could be R object names or code fragments that should be in backticks in the vignettes rather than whitelisted (e.g., afun, afuns, bbbs, splfun). Worth checking if wrapping them in backticks in the Rmd files removes the need for the wordlist entries

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

will look into this,s ome of those are real (bbbs stands for Behavioral Building Blocks, which is a term I use heavily in one of the vignettes to refer to the functions we pass to pre and post in make_split_fun, others do look like variable names though

Comment thread R/make_split_fun.R
Comment thread R/tree_accessors.R
)
}
attr(obj, "indent_mods") <- as.integer(value)
attr(obj, "indent_mods") <- rep(as.integer(value), length.out = length(obj))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is probably the right behavior, but it should be documented in NEWS since it changes the semantics of the setter

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is a bugfix rather than a semantics change because if you give it a single value something down stream (printing, I think) breaks, but still worth putting in the NEWS file

@Melkiades Melkiades left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks Gabe for the PR!! I added a couple of comments ^^

In general, I would suggest to add link to the issue and add the NEWS file when ready to review. If possible I would do bug fixes and docs improvement in different PRs, but it is not an issue

@gmbecker

gmbecker commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator Author

Sorry @Melkiades this probably should have been marked as a draft, but thanks for going through it and I will go through all your comments and respond to each (and likely make most suggested changes)

gmbecker and others added 4 commits June 4, 2026 09:09
Co-authored-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Gabe Becker <gabembecker@gmail.com>
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

badge

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/00tabletrees.R               894      89  90.04%   20, 72-76, 114, 117, 448, 539-540, 543, 705, 811, 938-939, 1041, 1044, 1046-1047, 1065-1068, 1088, 1203-1206, 1304-1309, 1472, 1573-1576, 1700-1703, 1740-1743, 1749-1754, 1814, 1821, 1917, 2029, 2042, 2045-2048, 2051-2054, 2084, 2117-2118, 2194-2219
R/as_html.R                    172      25  85.47%   5-10, 80, 152-157, 162-167, 182-186, 273
R/colby_constructors.R         626      36  94.25%   81, 134, 197-200, 267-270, 411, 427, 1211-1215, 1217-1221, 1302, 1391, 1552, 1591, 1602, 1610, 1613, 1638, 1659, 1805, 2028-2031
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         287      22  92.33%   272, 335-338, 349-350, 352, 354, 551-555, 619-622, 685-688
R/format_rcell.R                17       1  94.12%   47
R/indent.R                      13       2  84.62%   40-41
R/index_footnotes.R             66       0  100.00%
R/make_split_fun.R             166      30  81.93%   22-26, 36-39, 52-55, 58-61, 115, 119, 267, 270-273, 278-281, 366, 375, 377, 379, 430
R/make_subset_expr.R           137      14  89.78%   77-92, 169-177, 213, 302, 306, 315
R/summary.R                    144      38  73.61%   35, 80, 178-220, 269, 315-331, 366, 397
R/tree_accessors.R            1287     154  88.03%   110, 139-140, 264, 284, 310, 333, 363, 381, 400-404, 424, 446-449, 576, 603-604, 890-896, 1004, 1043, 1062, 1088, 1140, 1161, 1188-1189, 1202-1203, 1216-1217, 1226, 1262, 1297, 1335-1340, 1399, 1473-1477, 1495-1504, 1582, 1702-1705, 1730, 1752-1753, 1763, 1814, 1835-1840, 1861-1866, 1951, 1992, 2091, 2198, 2211, 2225, 2241, 2250, 2260-2264, 2314-2319, 2522, 2532-2535, 2545, 2570-2573, 2580, 2582-2585, 2707, 2741-2742, 2799, 3103, 3464, 3580, 3614-3639, 3730-3738, 3899, 3973-3979, 4284, 4408, 4493-4498, 4504, 4528-4533, 4581, 4606-4630, 4659-4665
R/tt_afun_utils.R              419      33  92.12%   60, 182, 189, 198-212, 280, 288-289, 507, 515-518, 600-604, 624, 638-640
R/tt_as_df.R                   400      23  94.25%   101-104, 112, 150, 224-227, 369, 388, 458, 477-480, 489, 599, 605, 637, 655, 707
R/tt_compare_tables.R           72       4  94.44%   51, 174, 249, 253
R/tt_compatibility.R           574      70  87.80%   22, 149-150, 193, 198, 329-330, 334-337, 343, 347, 531, 585-588, 625-627, 665, 698, 718, 738-741, 751-754, 799, 816-820, 826-829, 903, 930-933, 942, 1004, 1012, 1023-1026, 1137, 1144, 1172-1186, 1217-1218
R/tt_dotabulation.R           1266      97  92.34%   60, 255, 260, 262, 311, 336, 340-343, 376-379, 402, 435-438, 466-469, 596-597, 665, 852-856, 906, 910, 938-941, 951, 971-975, 982-985, 1249, 1253, 1284, 1388-1391, 1605-1613, 1877-1886, 1968-1971, 1982, 1987, 1992-1993, 1995, 2006, 2011, 2034, 2120-2139
R/tt_export.R                   13       1  92.31%   45
R/tt_from_df.R                  15       0  100.00%
R/tt_paginate.R                535      40  92.52%   74, 122-131, 242, 341-342, 494, 629-632, 653-657, 802-805, 856-863, 940, 943, 961, 968, 971
R/tt_pos_and_access.R          656      33  94.97%   76, 78-80, 105, 166, 262, 329, 438, 512, 516, 724, 726, 734, 740, 754, 764-767, 990, 1007-1010, 1037, 1096-1097, 1110, 1346-1347, 1373-1376, 1658, 1733
R/tt_showmethods.R             162      21  87.04%   56, 91-113, 223, 249, 258, 263, 266-270, 359-360
R/tt_sort.R                    115       6  94.78%   50, 289-292, 300
R/tt_toString.R                439      24  94.53%   125, 355, 377, 390, 400, 406, 409, 415-425, 518, 619, 826-851
R/utils.R                       34       7  79.41%   56, 169-174
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                         9002     846  90.60%

Diff against main

Filename              Stmts    Miss  Cover
------------------  -------  ------  -------
R/00tabletrees.R        +21     +21  -2.17%
R/tree_accessors.R       +7      +7  -0.48%
TOTAL                   +28     +28  -0.28%

Results for commit: ddde119

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Unit Tests Summary

    1 files     30 suites   1m 57s ⏱️
  249 tests   249 ✅ 0 💤 0 ❌
1 856 runs  1 856 ✅ 0 💤 0 ❌

Results for commit ddde119.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
Pagination 💔 $17.25$ $+1.11$ $0$ $0$ $0$ $0$
Result Data Frames 💔 $11.16$ $+1.12$ $0$ $0$ $0$ $0$
Tabulation framework 💔 $23.86$ $+1.91$ $0$ $0$ $0$ $0$

Results for commit 5809237

♻️ This comment has been updated with latest results.

@gmbecker

gmbecker commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator Author

superceded by #1093 I believe all of the review comments here have been addressed with the current state there.

@gmbecker gmbecker closed this Jun 4, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 4, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants