-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update merge_expression_module.R #93
Conversation
Code Coverage Summary
Results for commit: f7ffc3a Minimum allowed coverage is ♻️ This comment has been updated with latest results |
R/merge_expression_module.R
Outdated
@@ -335,7 +335,7 @@ merge_expression_srv <- function(id = "merge_id", | |||
merge_fun_name <- if (inherits(merge_function, "reactive")) merge_function() else merge_function | |||
check_merge_function(merge_fun_name) | |||
|
|||
ds <- Filter(Negate(is.null), lapply(selector_list(), function(x) x())) | |||
ds <- Filter(function(e) length(e) > 0, lapply(selector_list(), function(x) x())) |
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.
selector_list
is an output from the data_extract_multiple_srv and it's a list of selectors and it's hard to imagine that this line wouldn't remove null's already. So NULL and length == 0 is not possible for this list.
Let's focus on the possible look of the selector. Each selector is a list which contains:
- filters
- select
- always selected
- reshape <logical(1)>
- dataname <character(1)>
- internal_id <character(1)>
- keys
To make sure that you fix this please fix it to the following way
ds <- Filter(function(e) length(e) > 0, lapply(selector_list(), function(x) x())) | |
ds <- Filter(function(e) length(e$select) > 0, lapply(selector_list(), function(x) x())) |
If we do so, we can close this issue:
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.
This would require also:
- change the news file
- validate(need(length(ds) > 0, "Nothing selected"))
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.
Sorry, above suggestion and line in tmg where we removing selector with empty -select
slot is wrong.
We need to overcome this in different way
@@ -335,8 +335,8 @@ merge_expression_srv <- function(id = "merge_id", | |||
merge_fun_name <- if (inherits(merge_function, "reactive")) merge_function() else merge_function | |||
check_merge_function(merge_fun_name) | |||
|
|||
ds <- Filter(Negate(is.null), lapply(selector_list(), function(x) x())) | |||
validate(need(length(ds) > 0, "At least one dataset needs to be selected")) | |||
ds <- Filter(function(e) length(e$select) > 0, lapply(selector_list(), function(x) x())) |
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.
This would drop this variable from the $columns_source
, so it would fail some validation. We need to dig into merge_datasets and not merge selector which is not selected
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 am closing this PR as not has even an issue number.
I assume this will be a quick updated. FOr sure this have to be properly tested
No description provided.