472 Remove dm from main function
#473
Conversation
|
🧪 Code Coverage SummaryDiff against mainResults for commit: 18fdb97cd2b9031f210e334e9dd4ab4e591c341f Minimum allowed coverage is ♻️ This comment has been updated with latest results |
| dm_update_zoomed() | ||
| db | ||
| adam_db$adsl <- adam_db$adsl %>% | ||
| mutate(DOMAIN = "ADSL") |
There was a problem hiding this comment.
"domain" is a required variable in adsl, no need to update it here? or we don't want to make it a factor?
There was a problem hiding this comment.
so we need to add one level to the tree so that punning doesn't cause a bug when some data are missing. That was an old dirty trick, we can probably refactor the code in another issue.
There was a problem hiding this comment.
we should create a issue in rtables of this bug
There was a problem hiding this comment.
is split_rows_by("DOMAIN", split_fun = drop_split_levels, child_labels = "hidden") %>% used to create the indention previously? if so then we can safely remove that part
There was a problem hiding this comment.
and if you have anything in mind that could lead to this issue, please provide a example so that we can furhter follow up with rtables team
|
hi @BFalquet I think it is good chance to evaluate the checks we have for the templates, and unify a bit about creating "factors", and creating "no coding available" thing |
|
Refrain from merging until we get clarification from @crazycatandy (on 2023-04-06) |
|
i think in assert_all_tablenames finction, we make sure it is a named list of datasets, or a dm object. that won't break any code. the breaking change is in the filter system but we can try to mitigate it |
Since there are operations downstream that are not supported by |
I think they are not in main function, only in preprocessing right? since preprocessing are printted out then probably it is fine |
|
let's not make it draft, so that actions can be done automatically. No worries on the retain merging as i can onhold approval |
| #' iris = iris | ||
| #' ) | ||
| #' | ||
| #' assert_one_tablenames(lsd, c("mtcars", "x", "y"), qualifier = "first test:") | ||
| #' } | ||
| assert_one_tablenames <- function(db, tab, null_ok = TRUE, qualifier = NULL) { |
There was a problem hiding this comment.
what is this qualifier for?
There was a problem hiding this comment.
Not very important, it is just an argument to customize the error message. I don't think it is used anymore, we could remove it.
|
please also update the news |
|
sibling PR in |
There was a problem hiding this comment.
In general it looks good. dm is removed. I did not look into every template, but since the tests passed it should be fine. However, I spotted that there are some inconsistency in the code styles/programming practices in these templates. Though it is fine now, we should better update it later. some findings are summarized into #476 . Let's also create an issue to address these inconsistencies later.
the sibling PR of insightsengineering/chevron#473 - deprecate `dm_unite` and add a new `ls_unite` performing essentially the same on a list of data.frames. - deprecate `dm_explicit_na`. - change join_adsl_adsub into a method, deprecate its usage for dm and create a method for list of data.frame. - [x] adapt tests - [x] adapt pkgdown --------- Co-authored-by: benoit <benoit.falquet@roche.com>
#472
close #472
remove dm from
premainThis will cause breaking changes in
citrilthat can be addressed by imply convertingdmtolistofdata.framewithas.list.The following functions from dunlin are now useless:
dm_explicit_nadm_unitecorresponding PR in
dunlinto be merged at the same time:insightsengineering/dunlin#76
thank you for the review