-
Notifications
You must be signed in to change notification settings - Fork 62
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
Update test for new arrow release #764
Conversation
Oh, and I fixed an egregious set of typos in this test as well. 🙈 |
UGH, right, the Azure tests won't run on CI for some reason, as I was working on in #753. |
@thisisnic would you mind installing from this branch via |
@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. |
pin_write(board, df, "df-1", type = "rds") | ||
expect_equal(pin_read(board, "df-1"), df) | ||
|
||
pin_write(board, df, "df-2", type = "parquet") | ||
expect_equal(pin_read(board, "df-2"), df) | ||
|
||
pin_write(board, df, "df-3", type = "arrow") | ||
expect_equal(pin_read(board, "df-2"), df) | ||
expect_equal(pin_read(board, "df-3"), df) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a classic Hadley typo!
@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! 😄 Hopefully we won't have any more breaking changes for you again! 😬 |
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. |
The new pins release is on CRAN as of today, so hopefully you all are unblocked for whatever you need to do. 👍 |
Thanks! |
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. |
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 adata.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.