search

rstudio / pins-r

Pin, Discover and Share Resources
https://pins.rstudio.com
Other
301 stars 62 forks source link

Update test for new arrow release #764

Closed juliasilge closed 10 months ago

juliasilge commented 11 months ago

Closes #761

This PR changes the read 🔄 write tests to use tibble::tibble() to account for the new behavior in arrow, which now always returns a tibble on reading. If you write a tibble, only the CSV storage option now returns a data.frame. I went back and forth on how to handle this change, but I now think the best thing to do is to keep the same behavior in pins that arrow has.

This PR works with both old and new arrow, so we could do a release whenever. I tested locally with the devel version of arrow.

juliasilge commented 11 months ago

Oh, and I fixed an egregious set of typos in this test as well. 🙈

juliasilge commented 11 months ago

UGH, right, the Azure tests won't run on CI for some reason, as I was working on in #753.

juliasilge commented 10 months ago

@thisisnic would you mind installing from this branch via remotes::install_github("rstudio/pins-r@arrow-test-change") and confirming that this solves the problem for the development version of arrow, soon to go to CRAN? I have checked this but would appreciate one more person running it with the new arrow. Also let me know if you have any feedback!

thisisnic commented 10 months ago

@juliasilge Of course! CC'ing @paleolimbot who has a script for simplifying the arrow revdepcheck process who I'll need to talk to about this.

thisisnic commented 10 months ago

@juliasilge I've tested this with the new version of arrow on the pins main branch (tests fail, as expected), and then on the branch from this PR (tests pass), so looks good to me, thanks for updating! :smile: Hopefully we won't have any more breaking changes for you again! :grimacing:

juliasilge commented 10 months ago

Great news @thisisnic! I'll get this merged in.

I'm guessing you would prefer that we do a release pretty soon? I believe we don't have any blockers for that.

thisisnic commented 10 months ago

Great news @thisisnic! I'll get this merged in.

I'm guessing you would prefer that we do a release pretty soon? I believe we don't have any blockers for that.

We were planning to release the arrow R package to CRAN tomorrow, but this has been delayed by the top-level Arrow release being delayed too, so we're likely another week away. This is the first time I've had a "this release breaks a reverse dependency's tests"; I think ideally you'd release before us, but if not, we just get an email from CRAN telling us we broke your tests to which we just reply to let them knowing we've told you, and then I suppose they'd email you asking you to update soon? Sorry, that was long, tl;dr: yes please, but nothing catastrophic happens if we are slightly out of sync.

juliasilge commented 10 months ago

The new pins release is on CRAN as of today, so hopefully you all are unblocked for whatever you need to do. 👍

thisisnic commented 10 months ago

Thanks!

github-actions[bot] commented 10 months ago

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.