-
-
Notifications
You must be signed in to change notification settings - Fork 3
fix pipe #282
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 pipe #282
Conversation
Unit Tests Summary 1 files 24 suites 6s ⏱️ Results for commit 8433b47. ♻️ This comment has been updated with latest results. |
llrs-roche
left a comment
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.
Nice addition! I think this will help a lot, in the past I was manually adding it when I thought it was needed. Adding them automatically is a huge improvement.
I could test locally with the examples of merge_datasets() but that only covers the is.list() code branch but not the . Could you add test to cover all the function (the is.call() code branch too)?
Also note that this has the potential to break many snapshots that check for code on modules (but fix others :D).
Code Coverage SummaryDiff against mainResults for commit: 8433b47 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
osenan
left a comment
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.
LGTM.
I checked specialy call utils. The test are difficult for me to review. However, it is good that the checks, except lintr, pass.
Looks like merge module from teal.transform calls
%>%hereteal.transform/R/get_dplyr_call.R
Line 172 in 45d962e
and this leads to issues on CI/CD like in teal.modules.clinical, where teal_data does not attach
dplyrbut themagrittrordplyrare needed to use%>%, so we need to attach it inteal.transformhttps://github.com/insightsengineering/teal.modules.clinical/actions/runs/19731200858/job/56532457344