meztez / bigrquerystorage

R Client for BigQuery Storage API
Apache License 2.0
17 stars 1 forks source link

Remove `dbi.R` #50

Open hadley opened 2 months ago

hadley commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

57 confirms it. It has to disappear.