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

Reexport all of DBI #384

Closed
krlmlr opened this issue Jan 4, 2020 · 5 comments
Closed

Reexport all of DBI #384

krlmlr opened this issue Jan 4, 2020 · 5 comments

Comments

@krlmlr
Copy link
Collaborator

krlmlr commented Jan 4, 2020

Ideally we can use dbGetQuery() directly after loading duckdb. I wonder why the DBItest tests don't catch this.

library(duckdb)
dd <- data.frame(a = 1.5, b = "b")

con <- dbConnect(duckdb())
dbWriteTable(con, "dd", dd)

dbGetQuery(con, "SELECT round(a)")
#> Error in dbGetQuery(con, "SELECT round(a)"): could not find function "dbGetQuery"

dbGetQuery(con, "SELECT round(b)")
#> Error in dbGetQuery(con, "SELECT round(b)"): could not find function "dbGetQuery"

Created on 2020-01-04 by the reprex package (v0.3.0)

@hannes
Copy link
Member

hannes commented Jan 5, 2020

Good point, I always only load DBI and not duckdb. How would i go about re-exporting DBI? Can you send a PR perhaps?

@hannes hannes added the R label Jan 23, 2020
@jimhester
Copy link

I really don't think re-exporting the entire namespace is a good idea, this is what Depends is for...

@hannes
Copy link
Member

hannes commented Feb 5, 2020

@jmhester indeed that is the correct solution. Thanks for pointing this out! I have created a PR and will merge if no issues appear #411

@hannes
Copy link
Member

hannes commented Feb 5, 2020

@krlmlr
Perhaps DBItest could then check for that accordingly, right now I get

── 1. Failure: DBItest[duckdb]: Getting started: package_dependencies (@spec-get
3207"DBI" %in% pkg_imports isn't true.

So adding it into both Depends and Imports.

@hannes hannes closed this as completed Feb 5, 2020
@krlmlr
Copy link
Collaborator Author

krlmlr commented Feb 5, 2020

I'm not sure you can add to both Depends and Imports. Reexporting feels slightly neater than "Depends", and allows downstream packages to import only {duckdb} instead of both {duckdb} and {DBI}.

On the flip side, I see that "Depends" will make all methods (even new ones) available when users use library(duckdb), and is much less effort for the DBI driver. I'm not sure we need to update {DBItest} right away, let's keep this in mind.

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

3 participants