Conversation
Unit Test Performance Difference
Additional test case details
Results for commit 339cb5c ♻️ This comment has been updated with latest results. |
Code Coverage SummaryDiff against mainResults for commit: 228499e Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 10 suites 16s ⏱️ Results for commit 228499e. ♻️ This comment has been updated with latest results. |
| ## Overview | ||
|
|
||
| Every `picks()` object is built from three kinds of building blocks: `datasets()`, `variables()`, | ||
| and `values()`. Each one carries two core arguments: | ||
|
|
||
| - `choices` — the full set of options the app user can pick from. | ||
| - `selected` — the subset that is pre-selected when the app starts. | ||
|
|
||
| ``` | ||
| picks( | ||
| datasets(choices = <what datasets the user may choose>, selected = <default dataset>), | ||
| variables(choices = <what columns the user may choose>, selected = <default columns>), | ||
| values(choices = <what values the user may filter to>, selected = <default filter values>) | ||
| ) |
There was a problem hiding this comment.
I don't think we need a new vignette, since we already have a vignette with the overall information like this
There was a problem hiding this comment.
Merged to the main vignette here. On the other hand now the vignette is not as short as it used to be.
| Choices and selections can be expressed in three ways: | ||
|
|
||
| | Approach | Works with | | ||
| |---|---| | ||
| | Explicit — character vector, integer index, or numeric range | `datasets()`, `variables()`, `values()` | | ||
| | `tidyselect` helpers — `everything()`, `starts_with()`, `where()`, … | `datasets()`, `variables()` only | | ||
| | Function — an R function applied at runtime to the data | `datasets()`, `variables()`, `values()` | | ||
|
|
||
| The sections below walk through each approach with runnable examples. |
There was a problem hiding this comment.
Just move this information and the below to the main vignette
|
|
||
| ```{r data, message = FALSE} | ||
| library(teal.data) | ||
| library(teal.picks) |
There was a problem hiding this comment.
| library(teal.picks) |
There was a problem hiding this comment.
We need 1 of these for the vignette to build :-) it cannot be silent as in examples
There was a problem hiding this comment.
I removed anyway as the data is loaded by the main vignette
| resolver(x = p_datasets, data = as.list(data)) | ||
| resolver(x = p_variables, data = as.list(data)) | ||
| resolver(x = p_values, data = as.list(data)) |
There was a problem hiding this comment.
Is resolver an exported funtion? Do user need to know this function?
There was a problem hiding this comment.
We do not need it. Indeed it is confusing. I removed here. It is exported, btw.
m7pr
left a comment
There was a problem hiding this comment.
Overall looks good. I would only move this content to the main vignette. Also, I would not mention resolver. No need to call library(teal.picks) since this package is loaded when vignette is compiled.
| }) | ||
|
|
||
| join_keys(data) <- join_keys( | ||
| teal.data::join_key("ADSL", "ADLB", keys = "USUBJID") |
There was a problem hiding this comment.
These datasets are very incomplete, why not use teal.data::rADXX and teal.data::default_cdisc_join_keys[c("ADLB")]
Or add STUDYID to at least have the primary keys.
There was a problem hiding this comment.
I added the teal.data synthetic datasets, as you suggested here. I think it is an improvement
There was a problem hiding this comment.
I added the teal.data datasets as suggested. Here.
|
|
||
| | Approach | Works with | | ||
| |---|---| | ||
| | Explicit — character vector, integer index, or numeric range | `datasets()`, `variables()`, `values()` | |
There was a problem hiding this comment.
Consider adding a default row of what happens when defaults are assumed
|
|
||
| --- | ||
|
|
||
| ## 1. Explicit choices |
There was a problem hiding this comment.
Is explicit the right term?
User-defined, static, ?
Should it also reference what happens when choice doesn't exist
resolver(
teal.picks::picks(teal.picks::datasets("ADSL"), teal.picks::variables(c("yada"))),
teal.data::teal_data(ADSL = teal.data::rADSL)
)
#' warnings...
#' <picks>
#' <datasets>:
#' choices: ADSL
#' selected: ADSL
#' multiple=FALSE, ordered=FALSE, fixed=TRUE
#' <variables>:
#' choices: ~
#' selected: ~
#' multiple=FALSE, ordered=FALSE, fixed=TRUE, allow-clear=FALSE
averissimo
left a comment
There was a problem hiding this comment.
Almost there! 🥳
Take special look at the defaults for datasets() as I believe it's not consistent with API documentation and functinality
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
averissimo
left a comment
There was a problem hiding this comment.
Looks good!
Please add configuration to pkgdown to show articles index in main page.
There was a problem hiding this comment.
Please make a change in _pkgdown.yml to include all vignettes in site build
Adding a new vignette that explains how to select choices and selected values in picks datasets, variables and values