-
-
Notifications
You must be signed in to change notification settings - Fork 3
merge fix #83
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
merge fix #83
Conversation
| filtered_data_call <- lapply(selector_datanames, function(i) { | ||
| logger::log_trace("merge_datasets { paste0(i, \"_FILTERED\") } assigned.") | ||
| call( | ||
| "<-", | ||
| as.name(paste0(i, "_FILTERED")), | ||
| as.name(i) | ||
| ) | ||
| }) | ||
|
|
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.
there should be no filter calls in the merge_datasets as they are created in FilteredData.
| id = "merge_id") { | ||
| logger::log_trace("merge_expression_module called with: { paste(names(datasets), collapse = ', ') } datasets.") | ||
|
|
||
| checkmate::assert_list(data_extract, types = "data_extract_spec", names = "named") |
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.
To be compatible with:
teal.transform/R/data_merge_module.R
Line 103 in 801b8be
| checkmate::assert_list(data_extract) |
teal.transform/R/data_extract_module.R
Line 578 in 801b8be
| checkmate::assert_list(data_extract, names = "named") |
data_extract (multiple) can be a list of lists of data_extract_spec objects. Because single data_extract can be also a list
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.
Do we also need to check that if it is a list then that inner list is only data_extrac_specs?
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.
Yup, I've added more assertions
Code Coverage SummaryResults for commit: 8595e04 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
nikolas-burkoff
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.
A small comment but yup this looks good
| id = "merge_id") { | ||
| logger::log_trace("merge_expression_module called with: { paste(names(datasets), collapse = ', ') } datasets.") | ||
|
|
||
| checkmate::assert_list(data_extract, types = "data_extract_spec", names = "named") |
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.
Do we also need to check that if it is a list then that inner list is only data_extrac_specs?
|
@gogonzo I think this PR broke the merge and extract-merge vignettes in teal.transform |
Two fixes:
fix duplicated ADSL_FILTERED <- ADSL in the get_rcode

fix assertion of merge_expression_module to allow single data extract to be a list of extracts.