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 src_postgres #2881

Merged
merged 53 commits into from
Nov 8, 2021
Merged

Conversation

infotroph
Copy link
Member

@infotroph infotroph commented Nov 1, 2021

This is a continuation of PR #2785, which we can no longer update because of an accidentally deleted source branch that turned out to be more hassle to fix than seemed worth it. All the commits from #2785 are contained here and all comments from that discussion thread are now resolved.

Description

  • Removed all remaining calls to dplyr::src_postgres, mostly by replacing with PEcAn.DB::db.open() but sometimes with DBI::dbConnect() in places that are run without the PEcAn packages available. Fixes src_postgres needs to be deprecated everywhere #2688
  • Converted all bety <- db.open(...); con <- bety$con to con <- db.open(...)
  • Converted all bety$con to just bety (or con, in cases where bety was renamed in previous step)
  • Converted most existing DBI::dbConnect(...) calls to PEcAn.DB::db.open(...), except in contexts where PEcAn.DB may not be available (e.g. pecanapi demo) or to access non-PEcAn databases (e.g. some examples in DB package)
  • Consistently pass all of settings$database$bety into db.open() rather than pick out individual components. Attn @istfer: I believe this should allow port specification to work consistently as requested in src_postgres needs to be deprecated everywhere #2688 (comment)

Motivation and Context

dplyr::src_postgres() is now deprecated and the advice is to instead directly pass a DBI connection into dplyr::tbl(). We've previously replaced a few of these piecemeal, but not systematically (as evidenced by all the places the code was handling both bare con and bety$con).

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of .zenodo.json
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@dlebauer
Copy link
Member

dlebauer commented Nov 4, 2021 via email

@infotroph
Copy link
Member Author

modules/data.land/R/ens.veg.module.R Outdated Show resolved Hide resolved
modules/data.land/R/get.veg.module.R Outdated Show resolved Hide resolved
dbname='bety', driver='PostgreSQL',write=TRUE)
con <- PEcAn.DB::db.open(bety)
bety$con <- con
con <- PEcAn.DB::db.open(
Copy link
Contributor

@istfer istfer Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there isn't much to be done with these hardcoded ones (regarding the port I mean)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, these need to be addressed sometime but this refactor was sprawling far enough as it is :p

@infotroph
Copy link
Member Author

@robkooper Can you squash-merge this, please? The commits contain a lot of back and forth that doesn't need to be preserved.

@robkooper robkooper merged commit 25c5733 into PecanProject:develop Nov 8, 2021
@robkooper
Copy link
Member

done

@infotroph infotroph deleted the remove-src_postgres branch November 8, 2021 22:23
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.

src_postgres needs to be deprecated everywhere
5 participants