Skip to content
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

Returning list_of<> vs list in "low level" functions #660

Closed
DavisVaughan opened this issue Nov 12, 2019 · 1 comment · Fixed by #670
Closed

Returning list_of<> vs list in "low level" functions #660

DavisVaughan opened this issue Nov 12, 2019 · 1 comment · Fixed by #670

Comments

@DavisVaughan
Copy link
Member

I have a bit of a tough time deciding when to return a list_of<> from a vctrs function vs just a bare list. For example, vec_chop() returns a list_of<>:

vctrs::vec_chop(letters[1:2])
#> <list_of<character>[2]>
#> [[1]]
#> [1] "a"
#> 
#> [[2]]
#> [1] "b"

But this is slightly problematic because it is often used as a "low level" function where I really just want a bare list back. Like here in as_df_row_impl()

SET_ATTRIB(x, R_NilValue);

And here where I planned to implement vec_cast.list.data.frame

vec_cast.list.data.frame <- function(x, to, ...) {
  row.names(x) <- NULL
  
  # returns list_of()
  out <- vec_chop(x)
  attributes(out) <- NULL
  
  # alternative - much slower
  #out <- map(vec_seq_along(x), vec_slice, x = x)
  
  out
}

@lionel- and I think that vec_chop() should just return a bare list, but we would like to be consistent about this and make a systematic change in all vctrs functions. For example, would it still make sense for vec_split() to return a list_of<>?

vctrs::vec_split(letters[1:4], c(1, 1, 2, 2))$val
#> <list_of<character>[2]>
#> [[1]]
#> [1] "a" "b"
#> 
#> [[2]]
#> [1] "c" "d"

@hadley what do you think about this?

@hadley
Copy link
Member

hadley commented Nov 12, 2019

I don't have a strong preference; I think in general it's better to return the most specific type (which would be list_of<> here), but we have relatively little experience with list_of<>, so we probably don't want to use it in too many places. (And it provides relatively little benefit over list)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants