-
Notifications
You must be signed in to change notification settings - Fork 9
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
Remove dynamic domain determination #193
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
===========================================
- Coverage 100.00% 99.82% -0.18%
===========================================
Files 12 12
Lines 595 582 -13
===========================================
- Hits 595 581 -14
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. |
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.
Good job reflecting the domain changes across all the xportr functions. Left a few small comments. Will have a closer look at the vignette and docs after this.
@elimillera what is your ETA on this PR? We need to merge into #190 to finish this one off |
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 think this update warrants a note in the news/changelog!!
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.
There's astyler
CI errorMinor comment to update comments inR/*
- Should this change be added to
NEWS.md
? (it seems like a big breaking change for those that use this functionality)
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.
The tests/testthat/test-pipe.R
should be renamed as it no longer test pipes independently.
Some options:
- rename to
tests/testthat/test-domain.R
- same rename, but move
xporter_domain_name()
to its ownR/domain.R
- following
{testthat}
convention to test active file
- following
- move the tests to
tests/testthat/test-metadata.R
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 for the multiple review, but I've been noticing as I take a look at the assertion PR)
As it is, it allows for domain
to be NULL
with all the xportr
functions, which I think itsn't the desired behavior.
Previously it wasn't the case as it was fallbacking to the dynamic determination.
WDYT @elimillera ?
I actually think this is ok. If the user has really basic metadata that just has one domain. I think xportr is safe to assume that no filtering needs to be done for domain. I do see the arguments for requiring users explicitly supply it though |
I think some updates need to be made to the vignettes so the R-CMD checks pass |
Co-authored-by: Ben Straub <[email protected]>
Could you add the new function to the pkgdown site too? |
I was now testing all examples with empty domain and the "empty domain" problem appears on all other functions (with exception of 😕 Do you want to deal this on this PR or deal with this on a new one? Expand to see failing (reproducible) codelibrary(xportr)
#> ℹ Loading xportr
# format ----------------------------------------------------------------------
adsl <- data.frame(
USUBJID = c(1001, 1002, 1003),
BRTHDT = c(1, 1, 2)
)
metadata <- data.frame(
dataset = c("adsl", "adsl"),
variable = c("USUBJID", "BRTHDT"),
format = c(NA, "DATE9.")
)
adsl <- xportr_format(adsl, metadata)
#> Error in `dplyr::filter()`:
#> ℹ In argument: `dataset == domain & !is.na(format)`.
#> Caused by error:
#> ! `..1` must be of size 2 or 1, not size 0.
#> Backtrace:
#> ▆
#> 1. ├─xportr::xportr_format(adsl, metadata)
#> 2. │ └─metadata %>% ... at xportr/R/format.R:76:5
#> 3. ├─dplyr::filter(., !!sym(domain_name) == domain & !is.na(!!sym(format_name)))
#> 4. ├─dplyr:::filter.data.frame(., !!sym(domain_name) == domain & !is.na(!!sym(format_name))) at dplyr/R/filter.R:119:3
#> 5. │ └─dplyr:::filter_rows(.data, dots, by) at dplyr/R/filter.R:134:3
#> 6. │ └─dplyr:::filter_eval(...) at dplyr/R/filter.R:149:3
#> 7. │ ├─base::withCallingHandlers(...) at dplyr/R/filter.R:217:3
#> 8. │ └─mask$eval_all_filter(dots, env_filter) at dplyr/R/filter.R:217:3
#> 9. │ └─dplyr (local) eval() at dplyr/R/data-mask.R:99:7
#> 10. ├─dplyr:::dplyr_internal_error(...)
#> 11. │ └─rlang::abort(class = c(class, "dplyr:::internal_error"), dplyr_error_data = data) at dplyr/R/conditions.R:194:3
#> 12. │ └─rlang:::signal_abort(cnd, .file) at rlang/R/cnd-abort.R:379:3
#> 13. │ └─base::signalCondition(cnd) at rlang/R/cnd-abort.R:850:5
#> 14. └─dplyr (local) `<fn>`(`<dpl:::__>`)
#> 15. └─rlang::abort(message, class = error_class, parent = parent, call = error_call) at dplyr/R/conditions.R:235:5
# df_label --------------------------------------------------------------------
# adsl <- data.frame(
# USUBJID = c(1001, 1002, 1003),
# SITEID = c(001, 002, 003),
# AGE = c(63, 35, 27),
# SEX = c("M", "F", "M")
# )
#
# metadata <- data.frame(
# dataset = c("adsl", "adae"),
# label = c("Subject-Level Analysis", "Adverse Events Analysis")
# )
#
# adsl <- xportr_df_label(adsl, metadata)
# label -----------------------------------------------------------------------
adsl <- data.frame(
USUBJID = c(1001, 1002, 1003),
SITEID = c(001, 002, 003),
AGE = c(63, 35, 27),
SEX = c("M", "F", "M")
)
metadata <- data.frame(
dataset = "adsl",
variable = c("USUBJID", "SITEID", "AGE", "SEX"),
label = c("Unique Subject Identifier", "Study Site Identifier", "Age", "Sex")
)
adsl <- xportr_label(adsl, metadata)
#> Error in `dplyr::filter()`:
#> ℹ In argument: `dataset == domain`.
#> Caused by error:
#> ! `..1` must be of size 4 or 1, not size 0.
#> Backtrace:
#> ▆
#> 1. ├─xportr::xportr_label(adsl, metadata)
#> 2. │ └─metadata %>% dplyr::filter(!!sym(domain_name) == domain) at xportr/R/label.R:92:5
#> 3. ├─dplyr::filter(., !!sym(domain_name) == domain)
#> 4. ├─dplyr:::filter.data.frame(., !!sym(domain_name) == domain) at dplyr/R/filter.R:119:3
#> 5. │ └─dplyr:::filter_rows(.data, dots, by) at dplyr/R/filter.R:134:3
#> 6. │ └─dplyr:::filter_eval(...) at dplyr/R/filter.R:149:3
#> 7. │ ├─base::withCallingHandlers(...) at dplyr/R/filter.R:217:3
#> 8. │ └─mask$eval_all_filter(dots, env_filter) at dplyr/R/filter.R:217:3
#> 9. │ └─dplyr (local) eval() at dplyr/R/data-mask.R:99:7
#> 10. ├─dplyr:::dplyr_internal_error(...)
#> 11. │ └─rlang::abort(class = c(class, "dplyr:::internal_error"), dplyr_error_data = data) at dplyr/R/conditions.R:194:3
#> 12. │ └─rlang:::signal_abort(cnd, .file) at rlang/R/cnd-abort.R:379:3
#> 13. │ └─base::signalCondition(cnd) at rlang/R/cnd-abort.R:850:5
#> 14. └─dplyr (local) `<fn>`(`<dpl:::__>`)
#> 15. └─rlang::abort(message, class = error_class, parent = parent, call = error_call) at dplyr/R/conditions.R:235:5
# length ----------------------------------------------------------------------
adsl <- data.frame(
USUBJID = c(1001, 1002, 1003),
BRTHDT = c(1, 1, 2)
)
metadata <- data.frame(
dataset = c("adsl", "adsl"),
variable = c("USUBJID", "BRTHDT"),
length = c(10, 8)
)
adsl <- xportr_length(adsl, metadata)
#> Error in `filter()`:
#> ℹ In argument: `dataset == domain`.
#> Caused by error:
#> ! `..1` must be of size 2 or 1, not size 0.
#> Backtrace:
#> ▆
#> 1. ├─xportr::xportr_length(adsl, metadata)
#> 2. │ └─metadata %>% filter(!!sym(domain_name) == domain) at xportr/R/length.R:99:5
#> 3. ├─dplyr::filter(., !!sym(domain_name) == domain)
#> 4. ├─dplyr:::filter.data.frame(., !!sym(domain_name) == domain) at dplyr/R/filter.R:119:3
#> 5. │ └─dplyr:::filter_rows(.data, dots, by) at dplyr/R/filter.R:134:3
#> 6. │ └─dplyr:::filter_eval(...) at dplyr/R/filter.R:149:3
#> 7. │ ├─base::withCallingHandlers(...) at dplyr/R/filter.R:217:3
#> 8. │ └─mask$eval_all_filter(dots, env_filter) at dplyr/R/filter.R:217:3
#> 9. │ └─dplyr (local) eval() at dplyr/R/data-mask.R:99:7
#> 10. ├─dplyr:::dplyr_internal_error(...)
#> 11. │ └─rlang::abort(class = c(class, "dplyr:::internal_error"), dplyr_error_data = data) at dplyr/R/conditions.R:194:3
#> 12. │ └─rlang:::signal_abort(cnd, .file) at rlang/R/cnd-abort.R:379:3
#> 13. │ └─base::signalCondition(cnd) at rlang/R/cnd-abort.R:850:5
#> 14. └─dplyr (local) `<fn>`(`<dpl:::__>`)
#> 15. └─rlang::abort(message, class = error_class, parent = parent, call = error_call) at dplyr/R/conditions.R:235:5
# order -----------------------------------------------------------------------
adsl <- data.frame(
BRTHDT = c(1, 1, 2),
STUDYID = c("mid987650", "mid987650", "mid987650"),
TRT01A = c("Active", "Active", "Placebo"),
USUBJID = c(1001, 1002, 1003)
)
metadata <- data.frame(
dataset = c("adsl", "adsl", "adsl", "adsl"),
variable = c("STUDYID", "USUBJID", "TRT01A", "BRTHDT"),
order = 1:4
)
adsl <- xportr_order(adsl, metadata)
#> Error in `dplyr::filter()`:
#> ℹ In argument: `dataset == domain & !is.na(order)`.
#> Caused by error:
#> ! `..1` must be of size 4 or 1, not size 0.
#> Backtrace:
#> ▆
#> 1. ├─xportr::xportr_order(adsl, metadata)
#> 2. │ └─metadata %>% ... at xportr/R/order.R:95:5
#> 3. ├─dplyr::filter(., !!sym(domain_name) == domain & !is.na(!!sym(order_name)))
#> 4. ├─dplyr:::filter.data.frame(., !!sym(domain_name) == domain & !is.na(!!sym(order_name))) at dplyr/R/filter.R:119:3
#> 5. │ └─dplyr:::filter_rows(.data, dots, by) at dplyr/R/filter.R:134:3
#> 6. │ └─dplyr:::filter_eval(...) at dplyr/R/filter.R:149:3
#> 7. │ ├─base::withCallingHandlers(...) at dplyr/R/filter.R:217:3
#> 8. │ └─mask$eval_all_filter(dots, env_filter) at dplyr/R/filter.R:217:3
#> 9. │ └─dplyr (local) eval() at dplyr/R/data-mask.R:99:7
#> 10. ├─dplyr:::dplyr_internal_error(...)
#> 11. │ └─rlang::abort(class = c(class, "dplyr:::internal_error"), dplyr_error_data = data) at dplyr/R/conditions.R:194:3
#> 12. │ └─rlang:::signal_abort(cnd, .file) at rlang/R/cnd-abort.R:379:3
#> 13. │ └─base::signalCondition(cnd) at rlang/R/cnd-abort.R:850:5
#> 14. └─dplyr (local) `<fn>`(`<dpl:::__>`)
#> 15. └─rlang::abort(message, class = error_class, parent = parent, call = error_call) at dplyr/R/conditions.R:235:5
# type ------------------------------------------------------------------------
metadata <- data.frame(
dataset = "test",
variable = c("Subj", "Param", "Val", "NotUsed"),
type = c("numeric", "character", "numeric", "character"),
format = NA
)
.df <- data.frame(
Subj = as.character(123, 456, 789),
Different = c("a", "b", "c"),
Val = c("1", "2", "3"),
Param = c("param1", "param2", "param3")
)
df2 <- xportr_type(.df, metadata)
#> Error in `filter()`:
#> ℹ In argument: `dataset == domain`.
#> Caused by error:
#> ! `..1` must be of size 4 or 1, not size 0.
#> Backtrace:
#> ▆
#> 1. ├─xportr::xportr_type(.df, metadata)
#> 2. │ └─metadata %>% filter(!!sym(domain_name) == domain) at xportr/R/type.R:117:5
#> 3. ├─dplyr::filter(., !!sym(domain_name) == domain)
#> 4. ├─dplyr:::filter.data.frame(., !!sym(domain_name) == domain) at dplyr/R/filter.R:119:3
#> 5. │ └─dplyr:::filter_rows(.data, dots, by) at dplyr/R/filter.R:134:3
#> 6. │ └─dplyr:::filter_eval(...) at dplyr/R/filter.R:149:3
#> 7. │ ├─base::withCallingHandlers(...) at dplyr/R/filter.R:217:3
#> 8. │ └─mask$eval_all_filter(dots, env_filter) at dplyr/R/filter.R:217:3
#> 9. │ └─dplyr (local) eval() at dplyr/R/data-mask.R:99:7
#> 10. ├─dplyr:::dplyr_internal_error(...)
#> 11. │ └─rlang::abort(class = c(class, "dplyr:::internal_error"), dplyr_error_data = data) at dplyr/R/conditions.R:194:3
#> 12. │ └─rlang:::signal_abort(cnd, .file) at rlang/R/cnd-abort.R:379:3
#> 13. │ └─base::signalCondition(cnd) at rlang/R/cnd-abort.R:850:5
#> 14. └─dplyr (local) `<fn>`(`<dpl:::__>`)
#> 15. └─rlang::abort(message, class = error_class, parent = parent, call = error_call) at dplyr/R/conditions.R:235:5 Created on 2023-12-21 with reprex v2.0.2 |
I think we should handle this issue in this PR. |
Thanks for that @averissimo I made some updates and added some tests for that. It will just ignore the domain if it is null here. Is that more what you had in mind to fix it? |
@elimillera I saw the |
Changes Description
First draft of removing they dynamic determination of
.df
names. This is replaced with thedomain
argument and a new function for setting the df_arg attribute calledxportr_domain_name
. Closes #182Task List
devel
branch, Pull Requests tomain
should use the Release Pull Request Templatestyler
package and functions to style files accordingly.devtools::document()
so all.Rd
files in theman
folder and theNAMESPACE
file in the project root are updated appropriatelypkgdown::build_site()
and check that all affected examples are displayed correctly and that all new/updated functions occur on the "Reference" page.