Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 12 additions & 25 deletions R/parse.R
Original file line number Diff line number Diff line change
Expand Up @@ -82,31 +82,18 @@ parse_connectapi_typed <- function(data, ptype, strict = FALSE) {
}

parse_connectapi <- function(data) {
tibble::as_tibble(
purrr::list_rbind(
purrr::map(
data,
function(x) {
tibble::as_tibble(purrr::map(
.x = x,
.f = function(y) {
if (is.list(y)) {
# empty list object gets null
prep <- purrr::pluck(y, .default = NULL)
} else {
# otherwise NA
prep <- purrr::pluck(y, .default = NA)
}
if (length(prep) > 1) {
prep <- list(prep)
}
return(prep)
}
))
}
)
)
)
if (length(data) == 0) return(tibble::tibble())

all_names <- unique(unlist(lapply(data, names)))
cols <- stats::setNames(lapply(all_names, function(nm) {
values <- lapply(data, function(row) row[[nm]] %||% NA)
if (any(vapply(values, function(v) is.list(v) || length(v) > 1, logical(1)))) {
lapply(values, function(v) if (is.list(v)) v else list(v))
} else {
unlist(values)
}
}), all_names)
Comment thread
karawoo marked this conversation as resolved.
tibble::as_tibble(cols)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

SO much simpler!

This isn't part of this PR, but I wonder if this should even return a tibble at all, maybe that's something for the downstream consumers of this to deal with?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Are you saying we should return the list and have both parse_connectapi_typed and get_group_members convert to tibble? Or are you recommending converting to data.frame instead of tibble?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It wasn't a totally fleshed out thought. In the end I think returning lists at least internally will be easier to work with / not have as much overheard, but figuring out where the right point to do that is not totally trivial.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah, I generally agree

}

coerce_fsbytes <- function(x, to, ...) {
Expand Down
39 changes: 39 additions & 0 deletions tests/testthat/test-parse.R
Original file line number Diff line number Diff line change
Expand Up @@ -305,3 +305,42 @@ test_that("converts length one list", {
hm <- ensure_column(tibble::tibble(one = "hi"), NA_list_, "one")
expect_type(hm$one, "list")
})

test_that("parse_connectapi handles mixed null/non-null character values", {
data <- list(
list(guid = "aaa", bundle_id = NULL, name = "first"),
list(guid = "bbb", bundle_id = "123", name = "second")
)

result <- parse_connectapi(data)
expect_s3_class(result, "tbl_df")
expect_equal(nrow(result), 2)
expect_type(result$bundle_id, "character")
expect_identical(result$bundle_id, c(NA_character_, "123"))
})

test_that("parse_connectapi handles mixed null/non-null datetime strings", {
data <- list(
list(guid = "aaa", active_time = NULL),
list(guid = "bbb", active_time = "2023-08-22T14:13:14Z")
)

result <- parse_connectapi(data)
expect_s3_class(result, "tbl_df")
expect_equal(nrow(result), 2)
expect_type(result$active_time, "character")
expect_identical(result$active_time, c(NA_character_, "2023-08-22T14:13:14Z"))
})

test_that("parse_connectapi handles mixed null/non-null integer timestamps", {
data <- list(
list(key = "abc", start_time = 1732573574, end_time = NULL),
list(key = "def", start_time = 1732553145, end_time = 1732556770)
)

result <- parse_connectapi(data)
expect_s3_class(result, "tbl_df")
expect_equal(nrow(result), 2)
expect_type(result$end_time, "double")
expect_identical(result$end_time, c(NA_real_, 1732556770))
})
Loading