Skip to content

Commit

Permalink
fix vctrs issue
Browse files Browse the repository at this point in the history
  • Loading branch information
strengejacke committed Jun 3, 2024
1 parent 584bca9 commit 45499b1
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 12 deletions.
9 changes: 8 additions & 1 deletion R/utils_compact.R
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@
#' compact_list(c(1, NA, NA), remove_na = TRUE)
#' @export
compact_list <- function(x, remove_na = FALSE) {
# remove vctr-class attributes
if (is.data.frame(x)) {
x[] <- lapply(x, function(i) {
class(i) <- setdiff(class(i), c("haven_labelled", "vctrs_vctr"))
i
})
}
if (remove_na) {
x[!sapply(x, function(i) !is_model(i) && !inherits(i, c("Formula", "gFormula")) && (length(i) == 0L || is.null(i) || (length(i) == 1L && is.na(i)) || all(is.na(i)) || any(i == "NULL", na.rm = TRUE)))]
} else {
Expand All @@ -30,5 +37,5 @@ compact_list <- function(x, remove_na = FALSE) {
#'
#' @export
compact_character <- function(x) {
x[!sapply(x, function(i) nchar(i) == 0 || all(is.na(i)) || any(i == "NULL", na.rm = TRUE))]
x[!sapply(x, function(i) !nzchar(i, keepNA = TRUE) || all(is.na(i)) || any(i == "NULL", na.rm = TRUE))]
}
30 changes: 19 additions & 11 deletions tests/testthat/test-compact-list.R
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
test_that("compact_list works as expected", {
expect_equal(compact_list(list(NULL, 1, c(NA, NA))), list(1, c(NA, NA)))
expect_equal(compact_list(c(1, NA, NA)), c(1, NA, NA))
expect_equal(compact_list(list(NULL, 1, list(NULL, NULL))), list(1))
expect_equal(compact_list(c(1, NA, NA), remove_na = TRUE), 1)
expect_equal(compact_list(c(1, 2, 3), remove_na = TRUE), c(1, 2, 3))
expect_equal(compact_list(""), "")
expect_identical(compact_list(list(NULL, 1, c(NA, NA))), list(1, c(NA, NA)))
expect_identical(compact_list(c(1, NA, NA)), c(1, NA, NA))
expect_identical(compact_list(list(NULL, 1, list(NULL, NULL))), list(1))
expect_identical(compact_list(c(1, NA, NA), remove_na = TRUE), 1)
expect_identical(compact_list(c(1, 2, 3), remove_na = TRUE), c(1, 2, 3))
expect_identical(compact_list(""), "")
expect_null(compact_list(NULL))
expect_equal(compact_list(logical(0)), logical(0))
expect_identical(compact_list(logical(0)), logical(0))
})

test_that("compact_list, logical > 1", {
x <- list(a = 1, b = c(1, 2), c = NA)
expect_equal(compact_list(x, remove_na = TRUE), list(a = 1, b = c(1, 2)))
expect_equal(compact_list(x, remove_na = FALSE), list(a = 1, b = c(1, 2), c = NA))
expect_identical(compact_list(x, remove_na = TRUE), list(a = 1, b = c(1, 2)))
expect_identical(compact_list(x, remove_na = FALSE), list(a = 1, b = c(1, 2), c = NA))
x <- list(a = 1, b = c(NA, NA), c = NA)
expect_equal(compact_list(x, remove_na = TRUE), list(a = 1))
expect_equal(compact_list(x, remove_na = FALSE), list(a = 1, b = c(NA, NA), c = NA))
expect_identical(compact_list(x, remove_na = TRUE), list(a = 1))
expect_identical(compact_list(x, remove_na = FALSE), list(a = 1, b = c(NA, NA), c = NA))
})

test_that("compact_list, vctrs", {
data(mtcars)
class(mtcars$mpg) <- c("haven_labelled", "vctrs_vctr", "double")
attr(mtcars$mpg, "labels") <- c(`21` = 21)
out <- compact_list(mtcars)
expect_true(all(vapply(out, class, character(1)) == "numeric"))
})

2 comments on commit 45499b1

@bwiernik
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is an acceptable solution. The returned object needs to retain the vctrs classes.

@strengejacke
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that packages should not interfere with R language to such an extent that standard R code no longer works. ;-)
From some point of view, there's more benefit than harm when that class is removed, because it increases the probability that the returned objects works better with R / other packages than before, because it doesn't break regular R code.

But yes, if we find a solution to avoid my "hack", let's go that route.

Please sign in to comment.