voltrondata-labs / arrowbench

R package for benchmarking
Other
13 stars 9 forks source link

Deal with a purrr deprecation #125

Closed jonkeane closed 1 year ago

jonkeane commented 1 year ago

Adds as.list() (as suggested by the warning). The CI failures should all be resolved (and only are there because we depend on warnings being bubbled up)

alistaire47 commented 1 year ago

ah wait, I can't comment on this line, but I think this makes the missing check line 29 unnecessary because missing become explicit NULL

jonkeane commented 1 year ago

ah wait, I can't comment on this line, but I think this makes the missing check line 29 unnecessary because missing become explicit NULL

Sneaky! Lemme try and see. When I added that missing check I definitely added some tests to confirm it works

alistaire47 commented 1 year ago

Wait, I'm wrong; it doesn't coerce to NULL, it keeps the empty symbol, so I think we do still need that. It literally just changes the class from pairlist to list.

alistaire47 commented 1 year ago

Odd that missing symbols are of length 1. I guess it's viewed more as an empty string (or NA? but not really, since it can't be held in an atomic vector) than NULL.

But sorry for the deviation; this should be good as-is.

jonkeane commented 1 year ago

nods before I saw those comments there, I added a breakpoint and confirmed exactly what you're saying too:

Where forms is as.list(formals(FUN))

Browse[1]> str(formals(FUN))
Dotted pair list of 5
 $ one     : num 1
 $ a_few   : language c(1, 2, 3)
 $ null    : NULL
 $ a_vector: symbol known_sources
 $ none    : symbol 
Browse[1]> str(forms)
List of 5
 $ one     : num 1
 $ a_few   : language c(1, 2, 3)
 $ null    : NULL
 $ a_vector: symbol known_sources
 $ none    : symbol 
jonkeane commented 1 year ago

I'll merge when the CI is green