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

Remove dbi.R #50

Closed
hadley opened this issue Apr 11, 2024 · 4 comments
Closed

Remove dbi.R #50

hadley opened this issue Apr 11, 2024 · 4 comments

Comments

@hadley
Copy link
Contributor

hadley commented Apr 11, 2024

I think https://github.com/meztez/bigrquerystorage/blob/main/R/dbi.R doesn't belong in bigrquerystorage, because you shouldn't be defining methods if you don't own either the generic or the class. (It also makes it difficult for me to do the right thing in bigrquery, because as soon as bigrquerystorage is loaded, my methods are overridden)

@meztez
Copy link
Owner

meztez commented Apr 11, 2024

I understand some artifacts exist because bigrquerystorage was not a CRAN package to begin with.
They are helpful with the current CRAN released version of bigrquery. But, probably not for a future version of bigrquery.

How to avoid the incoming circular dependency thing?
bigrquerystorage relying on bigrquery for auth / bigrquery relying on bigrquerystorage for table download.

We could create a stripped down version of bigrquerystorage. Like an add-on to bigrquery, move bqs_auth/bqs_deauth, bqs_table_download, dbi. R to bigrquery.

I am not sure of the best approach to facilitate maintenance. In the meantime, I created a branch without dbi.R
https://github.com/meztez/bigrquerystorage/tree/nodbi

@hadley
Copy link
Contributor Author

hadley commented Apr 12, 2024

I don't know enough to have an opinion on the other bits yet, but I'm pretty sure overriding these methods is wrong. And I think it's ok to just remove them since you don't mention them in the readme.

I think it definitely makes sense for bqs_table_download() continue to live here.

@meztez
Copy link
Owner

meztez commented Apr 30, 2024

#57 confirms it. It has to disappear.

@hadley
Copy link
Contributor Author

hadley commented Sep 20, 2024

Done in #52

@hadley hadley closed this as completed Sep 20, 2024
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

No branches or pull requests

2 participants