Extend create_table()
#118
Conversation
|
Thanks for initiating this. Just my initial thoughts, I did not have so much time to check it: I thought of just changing Your method would probably safer and maybe the best way to go. But the one I mention above is overall less code. |
R/fct_appdata.R
Outdated
| var_levels <- dplyr::distinct(meta$items_expanded, item_name, item_group) | ||
| form_types <- lapply( | ||
| c("common_forms", "study_forms", "general"), | ||
| \(x) { | ||
| lapply(metadata[x], \(y) { | ||
| cbind(form_type = x, item_group = unique(y$item_group)) | ||
| }) |> | ||
| do.call(what = rbind) | ||
| }) |> | ||
| do.call(what = rbind) |> | ||
| as.data.frame() |
There was a problem hiding this comment.
Just a quick thought:
It might be cleaner if we add a small change to the items_expanded table when creating metadata. Instead of this:
clinsight/R/fct_data_helpers.R
Line 53 in 05b869c
just add an .id column:
dplyr::bind_rows(.id = "form_type")
Disadvantage is that we need to change the metadata, so it is a bit more invasive. What do you think?
There was a problem hiding this comment.
I'm not opposed. I think if we come up with more utility for the field that could make sense. I simplified the creation of the object a little bit by using Map() instead of nested lapply()s. I think it looks a bit less awkward now.
There was a problem hiding this comment.
If you are not opposed to it, maybe it is better to add it to the metadata$items_expanded table.
By doing so we don't even need to use lapply or Map in get_appdata.
In addition, I plan to remove the tabs common_forms, general, and study_forms from the metadata object in the future since it is redundant information (all this information is also in items_expanded)
There was a problem hiding this comment.
form_type column has been added to items_expanded
|
Before I forget: it would be nice to check if it is easy to add a custom common_form in the app_feature_04 integration test, so that this issue is covered |
Merge branch 'dev' into jt-107-extend_create_table # Conflicts: # NEWS.md
|
@LDSamson we good here? |
| @@ -267,9 +267,15 @@ get_appdata <- function( | |||
| "item_group consists of multipe elements which is not allowed: ", | |||
| item_group_x | |||
| ) | |||
| form_type_x <- unique(with(var_levels, form_type[item_group == item_group_x])) | |||
There was a problem hiding this comment.
Maybe it is good to wrap form_type_x also in simplify_string. That way the user is protected against some hard to detect changes in the names (for example, accidentally adding a leading or trailing white space in the table's name in the Excel file)
Merge branch 'dev' into jt-107-extend_create_table # Conflicts: # DESCRIPTION # inst/golem-config.yml
Merge branch 'dev' into jt-107-extend_create_table # Conflicts: # NEWS.md
|
Thanks! looks good |
Addresses #107