Closed akdor1154 closed 4 years ago
Merging #305 into master will increase coverage by
0.17%
. The diff coverage is88.88%
.
@@ Coverage Diff @@
## master #305 +/- ##
==========================================
+ Coverage 78.24% 78.42% +0.17%
==========================================
Files 6 6
Lines 754 760 +6
==========================================
+ Hits 590 596 +6
Misses 164 164
Impacted Files | Coverage Δ | |
---|---|---|
src/dbinterface.jl | 93.67% <87.50%> (+0.22%) |
:arrow_up: |
src/API.jl | 67.35% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update e1a98de...7f339f9. Read the comment docs.
thanks for the response, yep I had finished at that point but will fix up your review points in a little bit.
I am a bit confused by
I don't see the user/password fields removed from the Connection struct yet?
Have I missed/misunderstood something? I preserved
Connection(dsn::AbstractString, usr, pwd) = Connection(dsn; user=usr, password=pwd)
to not break your API but as far as i can tell user
and password
never actually end up in the Connection
struct (they are used to create the dbc
from API.Connect
/SQLDriverConnect
I guess)
Hey, This PR removes auth stuff from the DSN record of the Connection struct. Instead it is passed in at connection time them forgotten. Two rationales:
Additionally, it allows me to add an extraauth parameter which is appended as-is and likewise treated as sensitive info. This will be database specific, and could be used for e.g. auth tokens, Snowflake warehouse specification, etc etc, basically things for where my code's connection string needs to be the same per-user, but users may still need to put account-specific config to get a DB connection. (This model actually follows how PowerBI models ODBC connections, and I figure that they have done a bit more solution design across different DBs than I have :) )
Added public API:
Tests added and checked against MariaDB locally (we'll see what Travis says).. I will do further testing if Travis passes.
Any and all feedback welcome, this is my first public work in Julia so there are probably style issues at the very least.