-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Extend validate_has_data
to accept vector input
#962
Conversation
Code Coverage Summary
Diff against main
Results for commit: 9542b13 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.
I disagree with the applied solution.
validate_has_data
is a function specialized to deal with datasets, i.e. data.frame
s.
There is nothing wrong with the function, it was only used incorrectly by the module developer who passed a vector.
I think it is better to assert argument type here and fix the offending call (calls?) by adding drop = FALSE
. If a vector is passed by accident (as is clearly the case) the app should fail immediately and the bug should be caught in the console by the app developer, not in the app by a user.
Also, the changes to the documentation do not reflect changes to the functionality, and the changes to the code are more of a hack than an adjustment.
Thanks @chlebowa reverted the changes and now it only has an assert to check for Other places that use this function were |
Unit Test Performance Difference
Additional test case details
Results for commit 827755e ♻️ This comment has been updated with latest results. |
Co-authored-by: Aleksander Chlebowski <[email protected]> Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]> Signed-off-by: Vedha Viyash <[email protected]>
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.
👍
Closes insightsengineering/teal.modules.general#600