[WIP] Add derived subvar #453
Conversation
This reverts commit b5494f1.
|
R/add-subvariable.R:160:1: style: Variable and function names should not be longer than 30 characters. addSubvarsToSelectCatDerivation <- function(deriv, new_vars, existing_cats) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~R/add-subvariable.R:207:1: style: Lines should not be more than 100 characters. "cannot add subvariables.\n ", paste0(var_aliases, "(", cats_for_vars, ")", collapse = ", ")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~tests/testthat/test-add-subvariable.R:71:1: style: Lines should not be more than 100 characters. test_that("Cannot add new categories when adding existing variable to derived select_cat array", {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
Codecov Report
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 90.55% 90.11% -0.45%
==========================================
Files 121 121
Lines 6932 7048 +116
==========================================
+ Hits 6277 6351 +74
- Misses 655 697 +42
Continue to review full report at Codecov.
|
| derivation(variable) <- new_deriv | ||
| # We don't get metadata from original variable like we would if we were creating | ||
| # subvariable during original derivation... | ||
| # TODO: It would be nice to update name in the same POST as updating derivation |
There was a problem hiding this comment.
I think a derivation can have a subreferences field? Is that the best way?
R/add-subvariable.R
Outdated
| # subvariable during original derivation... | ||
| # TODO: It would be nice to update name in the same POST as updating derivation | ||
| # to ensure consistency, but I don't see how yet | ||
| # TODO: Also, the alias is really ugly for newly created subvariables |
There was a problem hiding this comment.
Is there a way to get the server's naming logic? I tried just adding the alias explicitly, hoping that the server would dedup for me, but this resulted in a failure.
|
|
||
| with_test_authentication({ | ||
| # adding to derived array | ||
| # TODO: Use existing fixture? Add this to fixtures? |
There was a problem hiding this comment.
This simple example works well for these tests, but we usually use newDatasetFromFixture(), worth changing to that (either by using existing fixture or adding this one there)?
|
R/add-subvariable.R:96:1: style: Lines should not be more than 100 characters. new_deriv <- addToSelectCatDerivation(old_deriv, subvariable, new_map_ids, categories(variable))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
|
R/add-subvariable.R:96:1: style: Lines should not be more than 100 characters. new_deriv <- addToSelectCatDerivation(old_deriv, subvariable, new_map_ids, categories(variable))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
Definitely not ready for primetime yet, but wanted to get some notes down.
Weird things:
absolutifyVariables(which modifies the derivation expression before user sees it) is wrong in some cases, likely having to do with array variables. It seems to assume that the variable is given by relative URL, but withinmapzcl function, variables are given by ID only. I have gotten around this by not converting to absolute paths, but I'd like to actually fix.