DAT-1561: Add functions for managing records in Databrary volume#6
Conversation
michalhuryn-montrose
left a comment
michalhuryn-montrose
left a comment
There was a problem hiding this comment.
LGTM, most of comments are cosmetic changes fix or leave, up to You.
localy some tests fails, verify if tests work at your side.
| if (is.null(response)) { | ||
| return(NULL) | ||
| } | ||
|
|
There was a problem hiding this comment.
In this method we don't cover success with empty body, shouldn't we expect that as well in responses?
| #' @eval options::as_params() | ||
| #' @name options_params | ||
| #' | ||
| NULL | ||
|
|
There was a problem hiding this comment.
I understand we had this pattern through all files in R package, but we could inherit those parameters from aaa.R file with R/assign_record_to_file.R:L21 #' @inheritParams options_params. So this should be moved to aaa.R and removed from all files that use inheritParams. This is just cosmetics.
| assertthat::assert_that(length(vol_id) == 1) | ||
| assertthat::assert_that(is.numeric(vol_id)) | ||
| assertthat::assert_that(vol_id >= 1) | ||
| assertthat::assert_that( | ||
| vol_id == floor(vol_id), | ||
| msg = "vol_id must be an integer" |
There was a problem hiding this comment.
Again patter which comes from previous implementation but could be addressed. Those few lines become a sort of validation boilerplate, we repeat this 4 times, maybe they can be extracted to some helper. Cosmetics, leave or change.
| #' | ||
| #' @param vol_id Target volume number. | ||
| #' @param category_id Category identifier. | ||
| #' @param vb Logical; if \code{TRUE}, print verbose messages. |
There was a problem hiding this comment.
we could inherit this from aaa.R
| vb = options::opt("vb"), | ||
| rq = NULL | ||
| ) { | ||
| # Validate vol_id |
There was a problem hiding this comment.
validation pattern, as before, I won't mention any later
|
|
||
| # Build request body (only include non-NULL fields) | ||
| body <- list() | ||
|
|
There was a problem hiding this comment.
trailing whitespace
| if (!is.null(measures)) { | ||
| body$measures <- measures | ||
| } | ||
|
|
There was a problem hiding this comment.
trailing whitespace
| if (!is.null(participant)) { | ||
| body$participant <- participant | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe we want to validate if we won't sent empty body with request.
| } | ||
|
|
||
| set_if_missing("DATABRARY_BASE_URL", "https://api.stg-databrary.its.nyu.edu") | ||
| set_if_missing("USER_AGENT", "SRW$*Kxy2nYdyo4LozoGV#i6LvH/") |
There was a problem hiding this comment.
Those secret's here are alarming. I think we should have CI/CD that test package, so we can get rid of all of them maybe. Also we miss info about needed env vars in Readme.md
michalhuryn-montrose
left a comment
michalhuryn-montrose
left a comment
There was a problem hiding this comment.
LGTM, additionaly I've added missing login_test_account() to failing tests.
…ion in snake_case_list function
DAT-1561 fix: add functions for enabling and disabling categories in Databrary volumes
2 participants
No description provided.