sunfuze / egg-knex

knex plugin for egg
MIT License
57 stars 15 forks source link

Allow specifying connectString #17

Closed cemremengu closed 6 years ago

cemremengu commented 6 years ago

In current implementation, connectString cannot be specified due to assertions.

codecov-io commented 6 years ago

Codecov Report

Merging #17 into master will decrease coverage by 0.49%. The diff coverage is 74.07%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #17     +/-   ##
======================================
- Coverage   75.49%   75%   -0.5%     
======================================
  Files           5     5             
  Lines         151   152      +1     
======================================
  Hits          114   114             
- Misses         37    38      +1
Impacted Files Coverage Δ
lib/knex.js 78.33% <74.07%> (-0.66%) :arrow_down:

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 6df9195...aa81fbc. Read the comment docs.

sunfuze commented 6 years ago

please add unit test code

cemremengu commented 6 years ago

@sunfuze It keeps rejecting the credentials (looks like it doesn't take the username into account?). I am not very familiar with mysql but I am using connectString way with oracle so it should work here as well.

Any ideas what may be wrong?

sunfuze commented 6 years ago

https://github.com/tgriesser/knex/blob/master/src/dialects/mysql/index.js#L67 this is how knex create mysql connection.

var connection = mysql.createConnection('mysql://user:pass@host/db?debug=true&charset=BIG5_CHINESE_CI&timezone=-0700');

this is example using connection string. so you should overwrite client.connection to connection string.

cemremengu commented 6 years ago

@sunfuze So looks like connectString option is only for oracledb. For everything else, like mysql, you specify

  client: {
    dialect: "mysql",
    connection: "mysql://root:@localhost/test"
  },

I think the best action here is to just remove the assertions and let the user specify whatever they want, which will fix everything . No need to assert anything since it will crash and tell the user what the error is.

If you agree, I will close this PR and open another one removing assertions for config.