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

tidy up dependencies #21

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

jangorecki
Copy link
Contributor

PR includes following changes:

  • move DBI from Depends to Imports in DCF:
    It is good practice to avoid Depends when possible in favor of Imports. Side effect is that we need to explicitly load DBI namespace if we want to use functions from it in examples
  • utils removed from DCF:
    No need to specify it. It is enough if it is in NAMESPACE
  • removed importFrom DBI in NAMESPACE:
    DBI was imported completely already in NAMESPACE so this line is redundant
  • test script correctly escapes suggested package:
    as per recommendation in WRE "...recommendation to use suggested packages conditionally in tests does also apply to packages used to manage test suites: a notorious example was testthat..."

@krlmlr
Copy link
Collaborator

krlmlr commented Oct 11, 2023

Thanks. Good catch on the @importFrom, I'll cherry-pick that.

The other points are valid, but also not very relevant. Shelving them for now.

@codecov-commenter
Copy link

Codecov Report

Merging #21 (42ea471) into main (cc4f7cb) will not change coverage.
Report is 2 commits behind head on main.
The diff coverage is n/a.

❗ Current head 42ea471 differs from pull request most recent head 61d7273. Consider uploading reports for the commit 61d7273 to get more accurate results

@@           Coverage Diff           @@
##             main      #21   +/-   ##
=======================================
  Coverage   85.63%   85.63%           
=======================================
  Files         106      106           
  Lines        3598     3598           
=======================================
  Hits         3081     3081           
  Misses        517      517           
Files Coverage Δ
R/csv.R 79.48% <ø> (ø)
R/dbConnect__duckdb_driver.R 100.00% <ø> (ø)
R/dbIsValid__duckdb_driver.R 100.00% <ø> (ø)
R/register.R 91.89% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

None yet

3 participants