enforce name uniqueness and pathability. unify section_div approach

[gmbecker]

This... ended up being more sizable than I expected to get all the behavior fixed and unified. Should be nearly backwards compatible with the exception of behavior that was buggy or otherwise incorrect before.

We should probably meet so I can walk you through this, but I'm happy to discuss on here instead/in addition.

[gmbecker]

Just realized I forgot to update the NEWS file, I will do that tomorrow.

[github-actions]

Unit Tests Summary

    1 files     28 suites   1m 45s ⏱️
  233 tests   233 ✅ 0 💤 0 ❌
1 725 runs  1 725 ✅ 0 💤 0 ❌

Results for commit c6a5a9e.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
Accessing and subsetting tables	💔	$4.17$	$+1.82$	$+46$	$0$	$0$	$0$
Accessor tests	💔	$2.27$	$+1.75$	$+37$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
Accessing and subsetting tables	👶		$+1.76$	tt_row_path_exists_and_tt_normalize_row_path_work
Accessor tests	💔	$0.40$	$+1.39$	section_div_getter_and_setter_works
Tabulation framework	👶		$+0.47$	path_uniqueness_sibling_name_uniqueness_is_enforced_correctly
sorting and pruning	💀	$0.17$	$-0.17$	sort_at_path_throws_an_error_when_trying_to_sort_a_table_with_identical_branching_names
sorting and pruning	👶		$+0.17$	sort_at_path_works_with_uniqified_names

Results for commit 088bddb

♻️ This comment has been updated with latest results.

[github-actions]

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/00tabletrees.R               812      63  92.24%   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, 1613-1616, 1652-1655, 1661-1666, 1717, 1724, 1818, 1926, 1939, 1942-1945, 1948-1951, 1978, 2010-2011
R/as_html.R                    172      25  85.47%   5-10, 77, 149-154, 159-164, 179-183, 270
R/colby_constructors.R         603      26  95.69%   81, 134, 197-200, 267-270, 411, 427, 1203, 1292, 1453, 1492, 1503, 1511, 1514, 1539, 1560, 1706, 1929-1932
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                13       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            1256     147  88.30%   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, 4239, 4363, 4448-4453, 4459, 4483-4488, 4536, 4561-4585, 4614
R/tt_afun_utils.R              417      33  92.09%   57, 178, 185, 194-208, 276, 284-285, 503, 511-514, 596-600, 620, 634-636
R/tt_as_df.R                   400      23  94.25%   100-103, 111, 149, 223-226, 368, 387, 457, 476-479, 488, 598, 604, 636, 654, 706
R/tt_compare_tables.R           72       4  94.44%   51, 174, 249, 253
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           1174      95  91.91%   60, 255, 260, 262, 311, 336, 340-343, 376-379, 402, 435-438, 466-469, 567, 709-713, 763, 767, 795-798, 808, 828-832, 839-842, 1106, 1110, 1141, 1244-1247, 1459-1467, 1731-1740, 1822-1825, 1836, 1841, 1846-1847, 1849, 1860, 1865, 1888, 1974-1993
R/tt_export.R                   13       1  92.31%   45
R/tt_from_df.R                  15       0  100.00%
R/tt_paginate.R                532      39  92.67%   74, 122-131, 336-337, 489, 624-627, 648-652, 797-800, 851-858, 935, 938, 956, 963, 966
R/tt_pos_and_access.R          650      39  94.00%   76, 78-80, 105, 166, 262, 329, 438, 512, 516, 724, 726, 734, 740, 754, 764-767, 953, 956, 964-968, 973-976, 1003, 1056-1057, 1068, 1307-1323, 1597, 1672
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                436      24  94.50%   126, 350, 372, 385, 395, 401, 404, 410-420, 513, 614, 821-846
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                         8724     807  90.75%

Diff against main

Filename                  Stmts    Miss  Cover
----------------------  -------  ------  -------
R/00tabletrees.R            +31       0  +0.31%
R/colby_constructors.R       +6       0  +0.04%
R/tree_accessors.R         +127      +5  +0.87%
R/tt_as_df.R                 +2       0  +0.03%
R/tt_paginate.R             +19      +2  -0.12%
R/tt_pos_and_access.R       +49      -2  +0.82%
TOTAL                      +234      +5  +0.20%

Results for commit: c6a5a9e

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[Melkiades]

Thank you @gmbecker for the PR!! It looks very good. I was thinking that we could make an addition to the vignette documentation showcasing the new/correct use of section_div and pathing. What do you think? Can be also a very small addition.

We could meet for an overview of the modification and additions. Wdyt @shajoezhu?
Could you check the lintr output too? It is only small things.

[gmbecker]

Hmm thats weird, I didn't commit until I had a clean local lint, I'll try updating lintr and running it agian and see if I can catch the ones that are tripping up the job there.

If you're happy with the PR we don't need to meet, but it ended up being more sizeable than I had expected/told Joe it would be so I'm happy to walk you through it if desired.

I can look at writing something into the vignette but I'm pretty crunched on time as we're in the process of opensourcing stuff on our side.

[gmbecker]

ugh, I updated lintr and it noticed some new things, but now I'm back to not being able to reproduce the lint results. its clean locally

> lint_package()
ℹ No lints found.
> getwd()
[1] "/Users/gbecker/gabe/checkedout/rtables"

[shajoezhu]

hi @gmbecker , I was wondering whats your lintr version?

[gmbecker]

It was old but I just updated it today in the process of prepping the second commit. It found issues my old version didnt but still doesn't match the one in the job apparently 😕

…

On Mon, May 19, 2025, 6:02 PM Joe Zhu ***@***.***> wrote: *shajoezhu* left a comment (insightsengineering/rtables#1024) <#1024 (comment)> hi @gmbecker <https://github.com/gmbecker> , I was wondering whats your lintr version? — Reply to this email directly, view it on GitHub <#1024 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG53MLZLC5B3HV4UFM547327J5KZAVCNFSM6AAAAAB5M3EMUOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQOJSGYYTANRTGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Melkiades]

@gmbecker it seems all a matter of indentation mismatch. Using {styler} should solve these issues. Spelling can be fixed too with the {spelling} package update

[gmbecker]

@Melkiades @shajoezhu styler and lintr are disagreeing:

> lint_package()
ℹ No lints found.
> library(styler)
> style_pkg()
Styling  87  files:
~<snip>~
──────────────────────────────────────────
Status	Count	Legend 
✔ 	83	File unchanged.
ℹ 	4	File changed.
✖ 	0	Styling threw an error.
──────────────────────────────────────────
Please review the changes carefully!
> sessionInfo()
R version 4.3.3 (2024-02-29)
Platform: aarch64-apple-darwin20 (64-bit)
Running under: macOS Sonoma 14.5

Matrix products: default
BLAS:   /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/lib/libRblas.0.dylib 
LAPACK: /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/lib/libRlapack.dylib;  LAPACK version 3.11.0

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

time zone: America/Los_Angeles
tzcode source: internal

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] styler_1.10.3 lintr_3.2.0  

loaded via a namespace (and not attached):
 [1] digest_0.6.34      backports_1.4.1    R6_2.5.1           xfun_0.41          magrittr_2.0.3     glue_1.8.0        
 [7] stringr_1.5.1      R.utils_2.12.3     knitr_1.45         rex_1.2.1          lifecycle_1.0.4    xml2_1.3.8        
[13] cli_3.6.2          R.methodsS3_1.8.2  vctrs_0.6.5        withr_3.0.2        compiler_4.3.3     rprojroot_2.0.4   
[19] R.oo_1.26.0        R.cache_0.16.0     purrr_1.0.2        rstudioapi_0.15.0  xmlparsedata_1.0.5 tools_4.3.3       
[25] lazyeval_0.2.2     roxygen2_7.3.2     rlang_1.1.4        stringi_1.8.3     

[shajoezhu]

reverse dependency check works ok https://github.com/insightsengineering/rtables/actions/runs/15151109225

[gmbecker]

@Melkiades @shajoezhu I would like adding to the vignette to be a separate issue which I can try to address in time for the release in early June but should not be a blocker for the release, as I did write pretty extensive docfiles for the various new bits. Does that work for you?

I think everything else has been addressed besides the annoying lintr v styler flamewar...

[Melkiades]

@Melkiades @shajoezhu I would like adding to the vignette to be a separate issue which I can try to address in time for the release in early June but should not be a blocker for the release, as I did write pretty extensive docfiles for the various new bits. Does that work for you?

I think everything else has been addressed besides the annoying lintr v styler flamewar...

I will take a look at the lintr issues. Besides for users, the vignette could be helpful also to discern new intended behavior and facilitate reviewing. This PR grew to have many features and changes, and it is a bit hard to keep track of them. Multiple smaller PRs are usually faster to review. Also snapshot testing with clear prompts could help reviewing too ^^. Joe and I are on it anyway!!

[shajoezhu]

hi @gmbecker , the PR looks good. We can come back for the vignette, once the spell and lintr issue is fixed, we can merge it in

[gmbecker]

@Melkiades @shajoezhu I would like adding to the vignette to be a separate issue which I can try to address in time for the release in early June but should not be a blocker for the release, as I did write pretty extensive docfiles for the various new bits. Does that work for you?
I think everything else has been addressed besides the annoying lintr v styler flamewar...

I will take a look at the lintr issues. Besides for users, the vignette could be helpful also to discern new intended behavior and facilitate reviewing. This PR grew to have many features and changes, and it is a bit hard to keep track of them. Multiple smaller PRs are usually faster to review. Also snapshot testing with clear prompts could help reviewing too ^^. Joe and I are on it anyway!!

I didn't expect it to get this large either, and I could have separated the section_div stuff out, I suppose, but it seemed natural to have them together and I wasn't sure wed have time to do both.

I think the tests I added/changed should give a pretty complete picture of the intended behavior.

I appreciate you guys working with me on this! :)

[shajoezhu]

lgtm! thanks @gmbecker , lets come back for the vignette

[shajoezhu]

downstream breaking change fixing in insightsengineering/autoslider.core@91393bd

insightsengineering/chevron#839

insightsengineering/tern#1422