Make gh_response play more nicely with vctrs / tidyr's rectangling toolkit

[jennybc]

I've been doing lots of API work lately with the master --> main task. And I've used the rectangling toolkit from tidy a lot. As it stands, I have to unclass() lists of gh responses everywhere, like so:

raw_pr_dat <- raw_prs %>%
  # strip the gh_response class to make tidyr happy about unnesting
  map(unclass) %>%
  enframe(name = "spec", value = "raw_prs") %>%
  rowwise() %>%
  filter(length(raw_prs) > 0) %>%
  unnest_longer(raw_prs)

I think the class should be c("gh_response", "list"). I got confirmation from @DavisVaughan that has been thought about over in vctrs and this is the recommended solution.

[gaborcsardi]

Yeah.

[jennybc]

It sure looks like this is already here, so now I need to understand why I am not enjoying the benefits:

gh/R/gh_response.R

Lines 31 to 37 in 60584ff

	if (is_ondisk) {
	class(res) <- c("gh_response", "path")
	} else if (is_raw) {
	class(res) <- c("gh_response", "raw")
	} else {
	class(res) <- c("gh_response", "list")
	}

[jennybc]

Yeah we already do this. I think the cause of the friction with the rectangling toolkit lies elsewhere. Closing for now.

> raw_prs %>%
+   .[c(1, 3)] %>% 
+   # strip the gh_response class to make tidyr happy about unnesting
+   #map(unclass) %>%
+   enframe(name = "spec", value = "raw_prs") %>% 
+   unnest_longer(raw_prs)
Error: Can't combine `..1$raw_prs` <gh_response> and `..2$raw_prs` <gh_response>.
x Some attributes are incompatible.
ℹ The author of the class should implement vctrs methods.
ℹ See <https://vctrs.r-lib.org/reference/faq-error-incompatible-attributes.html>.

Or maybe we have to implement some basic vctrs methods here ....

[jennybc]

I think the problem is that an instance of gh_response brings lots of metadata with it, via attributes, and that's what prevents them from being combined. Because the data in there varies.

> str(x$raw_prs, max.level = 1, list.len = 3)
List of 2
 $ :List of 16
  ..- attr(*, "method")= chr "GET"
  ..- attr(*, "response")=List of 26
  .. ..- attr(*, "class")= chr [1:2] "insensitive" "list"
  ..- attr(*, ".send_headers")= Named chr [1:3] "https://github.com/r-lib/gh" "application/vnd.github.v3+json" "token ghp_..."
  .. ..- attr(*, "names")= chr [1:3] "User-Agent" "Accept" "Authorization"
  ..- attr(*, "class")= chr [1:2] "gh_response" "list"
 $ :List of 5
  ..- attr(*, "method")= chr "GET"
  ..- attr(*, "response")=List of 26
  .. ..- attr(*, "class")= chr [1:2] "insensitive" "list"
  ..- attr(*, ".send_headers")= Named chr [1:3] "https://github.com/r-lib/gh" "application/vnd.github.v3+json" "token ghp_..."
  .. ..- attr(*, "names")= chr [1:3] "User-Agent" "Accept" "Authorization"
  ..- attr(*, "class")= chr [1:2] "gh_response" "list"

[DavisVaughan]

Yea, somewhat minimal reprex:

library(gh)
library(vctrs)

x <- gh("/users", .limit = 2)
Sys.sleep(1)
y <- gh("/users", .limit = 2)

vec_c(x, y)
#> Error: Can't combine `..1` <gh_response> and `..2` <gh_response>.
#> x Some attributes are incompatible.
#> ℹ The author of the class should implement vctrs methods.
#> ℹ See <https://vctrs.r-lib.org/reference/faq-error-incompatible-attributes.html>.

# Most obviously, `response$date` is different, but depending on the request I'd imagine
# you can have many differences
str(attributes(x), list.len = 3)
#> List of 4
#>  $ method       : chr "GET"
#>  $ response     :List of 27
#>   ..$ server                       : chr "GitHub.com"
#>   ..$ date                         : chr "Thu, 30 Sep 2021 18:45:43 GMT"
#>   ..$ content-type                 : chr "application/json; charset=utf-8"
#>   .. [list output truncated]
#>   ..- attr(*, "class")= chr [1:2] "insensitive" "list"
#>  $ .send_headers: Named chr [1:3] "https://github.com/r-lib/gh" "application/vnd.github.v3+json" "token ghp_..."
#>   ..- attr(*, "names")= chr [1:3] "User-Agent" "Accept" "Authorization"
#>   [list output truncated]
str(attributes(y), list.len = 3)
#> List of 4
#>  $ method       : chr "GET"
#>  $ response     :List of 27
#>   ..$ server                       : chr "GitHub.com"
#>   ..$ date                         : chr "Thu, 30 Sep 2021 18:45:45 GMT"
#>   ..$ content-type                 : chr "application/json; charset=utf-8"
#>   .. [list output truncated]
#>   ..- attr(*, "class")= chr [1:2] "insensitive" "list"
#>  $ .send_headers: Named chr [1:3] "https://github.com/r-lib/gh" "application/vnd.github.v3+json" "token ghp_..."
#>   ..- attr(*, "names")= chr [1:3] "User-Agent" "Accept" "Authorization"
#>   [list output truncated]

Since the attributes are different, vctrs doesn't know how to combine these things together nicely.

If you have a clear idea in your head of what vec_c(x, y) should do to reconcile these attributes correctly, then you could implement vctrs methods that would handle that.

Alternatively you could "declare" that <gh_response> + <gh_response> = <list>, and force vec_c(x, y) to return a bare list of the combined results with all classes and attributes removed (this is what c(x, y) does). It isn't an ideal solution, but I don't really know what the result of combining those should be, especially since each individual response might come from very different types of requests.

Even more complicated is the fact that you can have a gh_response that is a list under the hood, and another that is raw under the hood (or even a path apparently). No matter what solution is chosen above, I don't think it would solve combining a gh_response/list with a gh_response/raw. Maybe the class could be gh_response_list and gh_response_raw to differentiate them.

[DavisVaughan]

That would look something like:

library(gh)
library(vctrs)
library(tidyr)

x <- gh("/users", .limit = 2)
Sys.sleep(1)
y <- gh("/users", .limit = 2)

vec_c(x, y)
#> Error: Can't combine `..1` <gh_response> and `..2` <gh_response>.
#> x Some attributes are incompatible.
#> ℹ The author of the class should implement vctrs methods.
#> ℹ See <https://vctrs.r-lib.org/reference/faq-error-incompatible-attributes.html>.

vec_ptype2.gh_response.gh_response <- function(x, y, ...) {
  list()
}
vec_cast.list.gh_response <- function(x, to, ...) {
  attributes(x) <- NULL
  x
}

# working as well as can be expected
str(vec_c(x, y), list.len = 3)
#> List of 4
#>  $ :List of 18
#>   ..$ login              : chr "mojombo"
#>   ..$ id                 : int 1
#>   ..$ node_id            : chr "MDQ6VXNlcjE="
#>   .. [list output truncated]
#>  $ :List of 18
#>   ..$ login              : chr "defunkt"
#>   ..$ id                 : int 2
#>   ..$ node_id            : chr "MDQ6VXNlcjI="
#>   .. [list output truncated]
#>  $ :List of 18
#>   ..$ login              : chr "mojombo"
#>   ..$ id                 : int 1
#>   ..$ node_id            : chr "MDQ6VXNlcjE="
#>   .. [list output truncated]
#>   [list output truncated]

df <- tibble(col = list(x, y))

unnest_longer(df, col)
#> # A tibble: 4 × 1
#>   col              
#>   <list>           
#> 1 <named list [18]>
#> 2 <named list [18]>
#> 3 <named list [18]>
#> 4 <named list [18]>

^{Created on 2021-09-30 by the reprex package (v2.0.1)}

[jennybc]

Dropping the attributes when combining sounds reasonable to me.

[jennybc]

And, yes, as you say @DavisVaughan, this looks goofy now, when viewing gh_response through the lens of vctrs:

gh/R/gh_response.R

Lines 31 to 37 in 60584ff

	if (is_ondisk) {
	class(res) <- c("gh_response", "path")
	} else if (is_raw) {
	class(res) <- c("gh_response", "raw")
	} else {
	class(res) <- c("gh_response", "list")
	}

[gaborcsardi]

At this point, I think the best we can do is to document how to drop the attributes, with some examples.

[hadley]

@gaborcsardi Given that we're likely to break other minor stuff with the switch to httr2, do you think it's ok to change this now?

[gaborcsardi]

It is unclear to me now what we would need to do exactly. Do we have to always drop the attributes from gh_response? That does not sound too good.

[hadley]

@gaborcsardi I think we can drop the attributes whenever we combine two gh objects together. I think it's unlikely that would cause you to lose attributes that you actually care about.

[gaborcsardi]

True, but in the original example gh was not doing the combining. So can gh do anything there? Maybe a c() method is called downstream? Or can we define another method to make that work?

[hadley]

@gaborcsardi we could just supply the vctrs methods that @DavisVaughan suggested.

[gaborcsardi]

Oh, good, sure, we can do that.