Skip to content
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

Add readr to Imports #462

Merged
merged 4 commits into from
Sep 28, 2021
Merged

Add readr to Imports #462

merged 4 commits into from
Sep 28, 2021

Conversation

AdeelK93
Copy link
Contributor

I have my own package that I'm working on, and it happens to include a R CMD CHECK test that calls a bigrquery function.

This test has been failing because bigrquery has readr in the Suggests section of the DESCRIPTION rather than the Imports. Fortunately this is a quick fix.

Here is an example of where readr is used in bigrquery:
https://github.com/r-dbi/bigrquery/blob/main/R/bq-download.R#L340

@jennybc
Copy link
Collaborator

jennybc commented Sep 28, 2021

The function that you point out (bq_parse_file()) was originally commented as one of the "Helpers for testing". But we are now using it to parse the first chunk obtained in bq_table_download() (as of 1.4.0, thanks to my PR #456). Apparently I didn't realize that it made unconditional calls to a suggested package (readr).

@hadley Do you have a sense how you want to handle this? I suspect importing readr is not the right solution. We're just using readr::read_file(), presumably for pragmatic reasons.

@hadley
Copy link
Member

hadley commented Sep 28, 2021

Probably the easiest fix would be to use brio::read_file() instead; it should have similar performance characteristics and is a low-dependency package.

@jennybc
Copy link
Collaborator

jennybc commented Sep 28, 2021

@AdeelK93 can you update this PR to:

  • Not import readr.
  • Import brio. Please note that we alphabetize such package-listing sections of DESCRIPTION.
  • Replace calls to readr::read_file() with brio::read_file()

@AdeelK93
Copy link
Contributor Author

Thanks @jennybc @hadley.
I've made those changes, and left readr in Suggests

@jennybc
Copy link
Collaborator

jennybc commented Sep 28, 2021

@hadley Shall we do a patch release?

FWIW the only remaining uses of readr are via the deprecated list_tabledata() function:

if (!requireNamespace("readr", quietly = TRUE)) {
stop("Must install readr package to use `list_tabledata", call. = FALSE)
}

bigrquery/R/old-tabledata.R

Lines 214 to 222 in 1a549b0

time = function(x) {
readr::parse_time(x, "%H:%M:%OS")
},
date = function(x) {
readr::parse_date(x, format = "%Y-%m-%d")
},
datetime = function(x) {
readr::parse_datetime(x)
}

@jennybc jennybc merged commit 504185d into r-dbi:main Sep 28, 2021
@AdeelK93 AdeelK93 deleted the patch-1 branch September 29, 2021 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants