-
Notifications
You must be signed in to change notification settings - Fork 50
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
Fix potential tt at path issue #794
Conversation
Code Coverage Summary
Diff against main
Results for commit: 8b468b3 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Liming for finding this bug! I could not reproduce it in base {rtables} but I corrected the more general row_paths()
that is based on make_rows_df()
so now it will not produce named paths. I checked with surv_time
and cell_values
which was also breaking and the latter fix works. I think it is fine not to have identical
as the label is not really a needed check. I added a test but it is not generating named paths. @edelarua do you know how it is possible to correct the test to have it doing what surv_time
does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the test - this is good to go now!
there is a fix for path names
rtables/R/index_footnotes.R
Line 127 in 478e637
however tt_at_path is still not updated. It is not robust enough.
it will give you an error that the path is not valid.
Let's fix this issue from the root. Let's not use "identical" for comparison (to avoid potential issue of character names), or let's just use unname() for all occurences of row_paths, tt_at_path, etc. ideally the row_paths should not have any name.