Fixes validation and case when worst flag does not exist - #1543
Conversation
Unit Test Performance DifferenceTest suite performance difference
Additional test case details
Results for commit 36a90d8 ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
LGTM, I tested many validations and they worked. Please review the check of the parentname before merging. It seems there is an error when we are running some tests. https://github.com/insightsengineering/teal.modules.clinical/actions/runs/27220598240/job/80374092259
|
|
||
| y$table <- substitute( | ||
| expr = { | ||
| stopifnot("No common arm levels in data, please check input." = nrow(parent) > 0) |
There was a problem hiding this comment.
@osenan non-outdated code here
Do we need this protection? I'm worried about other consequences in other modules.
It was moved to the module to prevent generic error. This is a minor change to protect against a never ending calculation of rtables
There was a problem hiding this comment.
Just to clarify. This line is not existing in the code right now, you are asking if it is required. IMO it is not required. I tried to create an example of this scenario to fail but I could not found a case where the app works. Before the app runs the join keys always fails If I filter and keep the parent dataset without rows. It always fails so I expect this protection to be really an edge case that it is not necessary
osenan
left a comment
There was a problem hiding this comment.
It looks good! We only have to fix one typo
|
|
||
| y$table <- substitute( | ||
| expr = { | ||
| stopifnot("No common arm levels in data, please check input." = nrow(parent) > 0) |
There was a problem hiding this comment.
Just to clarify. This line is not existing in the code right now, you are asking if it is required. IMO it is not required. I tried to create an example of this scenario to fail but I could not found a case where the app works. Before the app runs the join keys always fails If I filter and keep the parent dataset without rows. It always fails so I expect this protection to be really an edge case that it is not necessary
Co-authored-by: Oriol Senan <35930244+osenan@users.noreply.github.com> Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
osenan
left a comment
There was a problem hiding this comment.
We can include the data protection, but take into account that currently is not in the branch, is only a comment on the PR
714fd08
into
fix-tm_abnormaliry@279-interactive_variables@main

Pull Request
Changes description
teal's validation input frameworkExample app (inherited from other PR)