Closed cdubz closed 5 years ago
coveralls
commented
5 years ago | Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| data/dynamodb/dynamodb.go | 157 | 20.33% | ||
| <!-- | Total: | 157 | --> |
| Totals | |
|---|---|
| Change from base Build 120: | -14.9% |
| Covered Lines: | 572 |
| Relevant Lines: | 1207 |
haydenwoodhead
commented
5 years ago First off thanks for the PR.
I've noticed that travis is failing due to some of my errors so I'll fix those up. Then you can rebase this branch on dev again.
As far code duplication goes it could work to have a more general sql db implementation and have a postgres and sqlite (note mysql wont work here due to a different variable syntax) files that simply import the appropriate driver, connect to the db and give the resulting sqlx.DB to the general sql db type.
Thoughts on this? Would you be interested in doing this? Otherwise I am happy to merge this in with a few changes.
cdubz
commented
5 years ago Your thinking re: a more general sql implementation makes sense to me. I'd be happy to take this on if you don't mind being a little patient with me (still very new to Go). Should we work on that in this PR or a separate one?
haydenwoodhead
commented
5 years ago Yeah absolutely go ahead and do the general implementation. Just work off this pr and we can can tidy up the git history when we're both happy with it.
I'm also going to look into why dynamodb is still failing on this pr. It's not failing for me on dev branch.
cdubz
commented
5 years ago Ok, here is a start, I think -- using an SQLDatabase struct that both SQLite3 and PostgreSQL inherit from. Tests for both drivers pass locally for me, and there is probably some opportunity to simplify the testTTLDelete for each as well...
cdubz
commented
5 years ago Yes, I realized I didn't test a full build right after pushing, hah. I think the lib/pq and mattn/go-sqlite3 drivers go get registered. I tested this with the sqlite3 driver, at least. I may just need to update the switch statement in config.go. Either way, I'll get that sorted next.
Taking me a while to wrap my head around Go's inheritance coming from OOP in Python and PHP...
cdubz
commented
5 years ago Ok! Re-added the GetPostgreSQLDB and GetSQLite3DB functions as wrappers, which is obviously much cleaner than my initial attempt to resolve this (:
haydenwoodhead
commented
5 years ago Right this looks great. Can I just ask one more thing then LGTM! Could you do an interactive rebase and squash down some of your fix up commits to make the git history a bit cleaner?
cdubz
commented
5 years ago Rebased and squashed!
haydenwoodhead
commented
5 years ago Awesome thanks so much. I'm going to work on adding a special cgo enabled build in makefiles in the next few days so lookout for that.
This PR adds support for SQLite3 with mattn/go-sqlite3. According to the SQL database drivers documentation there are no sqlite3 drivers in pure Go, so this change would require enabling CGO (see
build.sh:37)).Also, this implementation is almost exactly the same as the pgsql one, so probably a lot of room for improvement/sharing between the two.