Conversation
Tested with readr 2.1.0 (minimum in our DESCRIPTION) and this works. Must be a change in vroom, otherwise this error would have been thrown earlier.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #291 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 650 650
=========================================
Hits 650 650 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@PietrH, any advice? |
Vroom currently enforces an indirect dependency of R>4.1 on frictionless-r: https://github.com/tidyverse/vroom/blob/cd07f44dc51ddcb0795ccda84c814c8af75668df/DESCRIPTION#L28-L29 So while there are a few options to get around this test failure, it's probably most defensive/easiest to increase the minimum R requirement of frictionless-r. If you don't want to do this, I'm afraid we'll have to implement some sort of fallback for R 3.6 to download and read from disk instead. |
|
Updated to R > 4.1.0. All tests pass now. @PietrH can you review? |
PietrH
left a comment
There was a problem hiding this comment.
Looks good! 👍
Minor nitpick: we could consider requiring vroom 1.3.0 as a minimum. It's possible (but unlikely) for a user to update frictionless-r, but not keeping an older version of vroom that they had installed already.
It's also possible to step around this by using rlang::check_installed("vroom", version = "1.3.0"), you could even do this conditionally just in case an archive is read. That way you keep supporting older versions (of both R and vroom). Seems like a hassle though.
Personally I think we should either require vroom 1.3.0, or just count on users keeping a relatively updated R library.
|
Yeah, I agree that working conditionally is too much of a hassle. Also, I don't think vroom >= 1.3.0 is necessary. Users who don't update will experience the same behaviour as before (remote zips are not supported, a meaningful error is shown). The R >= 4.1.0 requirement is mainly important for our CI. |
We had a test that expects an error for when reading remote zips (based on this comment in readr). Seems like that is supported now, not because of change in readr, but in vroom: https://github.com/tidyverse/vroom/releases/tag/v1.7.0
This update would have thrown errors on CRAN, but it has
skip_if_offline(). Still, best to remove it for our CI/CD.