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

Get bind_rows to work with tbl_json objects #58

Closed
jeremystan opened this issue Sep 3, 2016 · 4 comments
Closed

Get bind_rows to work with tbl_json objects #58

jeremystan opened this issue Sep 3, 2016 · 4 comments

Comments

@jeremystan
Copy link
Collaborator

You can then get rid of rbind_tbl_json in utils.R

@jeremystan
Copy link
Collaborator Author

Not sure how to do this, given bind_rows doesn't work like other dplyr functions, wrap_dplyr_verb won't work to naturally extend it.

@colearendt
Copy link
Owner

colearendt commented May 22, 2017

bind_rows is not an S3 method, so it is not dispatched based on class. This makes sense in one regard, because it would require "aggregating" classes across many objects.

Likely some discussion needed on what implementation is suggested for tidyjson. There is relevant discussion on this currently open issue in dplyr. The discussion is promising of S3 behavior in the future.

Not sure whether it is better to create another verb with explicit tidyjson behavior (i.e. jbind_rows or something), or if masking the bind_rows function from dplyr makes more sense. The latter has the benefit of making things nice for those who load tidyjson after dplyr. Of course, for those who do not, tidyjson::bind_rows will be required.

A proposed implementation, based on the latter and drawing heavily from dplyr. Not sure if a warning is warranted or not:

bind_rows <- function(...) {
  r <- dplyr::bind_rows(...)
  
  d <- list_or_dots(...)
  if (all(unlist(lapply(d,is.tbl_json)))) {
    j <- unlist(lapply(d, attr, 'JSON'), recursive=FALSE)
    return(tbl_json(r,j))
  } else {
    warning('Some non-tbl_json objects.  Reverting to dplyr::bind_rows')
    return(r)
  }
}

See this commit

@colearendt
Copy link
Owner

Potential danger of implementing this before S3 support in dplyr is whether or not tidyjson will need to reexport the bind_rows once an S3 method is provided, for backwards compatibility.

@colearendt
Copy link
Owner

Closed in #108 and #109 , with the caveats above.

When this lands, we should revisit: tidyverse/dplyr#2457

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

No branches or pull requests

2 participants