Decouple scda #858
Decouple scda #858
Conversation
|
Code Coverage SummaryDiff against mainResults for commit: 25c8435 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
R/dummy_functions.R
Outdated
| ADSL$SEX[1:150] <- NA # nolint | ||
| ADSL$SEX[c(2, 5)] <- NA # nolint | ||
|
|
||
| cdisc_data_obj <- cdisc_data( |
There was a problem hiding this comment.
| cdisc_data_obj <- cdisc_data( | |
| cdisc_data_obj <- teal.data::cdisc_data( |
There was a problem hiding this comment.
Even though teal.data is in Depends?
There was a problem hiding this comment.
just to stay consistent with the code nearby
| @@ -111,41 +111,41 @@ | |||
| #' @include modules.R | |||
| #' | |||
| #' @examples | |||
There was a problem hiding this comment.
Hmmm still thinking about the example for this init() function...
To me, most of the people looking at this example would came from clinical trials domain so it would be good to speak in the same language - i.e. use ADAM model. WDYT @lcd2yyz ?
There was a problem hiding this comment.
I can copy paste data created in get_dummy_data so we can have an ADaM alike data here.
There was a problem hiding this comment.
There is also vignette including-adam-data-in-teal.Rmd for people who want to use teal in clinical trials domain. I'd prefer to keep teal::init documentation generic.
| ADSL <- data.frame( # nolint | ||
| STUDYID = "study", | ||
| USUBJID = 1:10, | ||
| SEX = sample(c("F", "M"), 10, replace = TRUE), | ||
| AGE = stats::rpois(10, 40) | ||
| ) |
There was a problem hiding this comment.
if it happens to repeat multiple times across the codebase (see my other comment about example in init() and also looking at the vignette) - please consider yet another (not so sure if private) object in the package namespace but please don't name it a "dummy" :P - make it "ex_adsl" or something like that.
There was a problem hiding this comment.
@pawelru it's a private function, used only in tests and documentation of other internal functions. Do you want this to still change it to ex_cdisc_data or something else?
There was a problem hiding this comment.
only if it happens that this is being defined multiple times
There was a problem hiding this comment.
@pawelru I don't want to export example data function just for a sake of one publicly available examples. I think it make sense to show also cdisc_data() call when calling teal::init.
There was a problem hiding this comment.
I've changed dummy_* to example_* and kept them as unexported
- [ ] teal insightsengineering/teal#858 - [ ] teal.widgets - [ ] osprey insightsengineering/osprey#121 - [ ] teal.osprey insightsengineering/teal.osprey#214 - [ ] teal.transform insightsengineering/teal.transform#139 --------- Signed-off-by: Dawid Kałędkowski <[email protected]> Co-authored-by: kartikeya kirar <[email protected]>
- [ ] teal insightsengineering/teal#858 - [ ] teal.widgets - [ ] osprey insightsengineering/osprey#121 - [ ] teal.osprey insightsengineering/teal.osprey#214 - [ ] teal.transform insightsengineering/teal.transform#139
closes #834
see also: