Skip to content
This repository has been archived by the owner on Dec 28, 2023. It is now read-only.

Allow specifying connectString #17

Closed
wants to merge 13 commits into from

Conversation

cemremengu
Copy link
Contributor

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

@codecov-io
Copy link

codecov-io commented May 11, 2018

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%) ⬇️

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
Copy link
Owner

sunfuze commented May 13, 2018

please add unit test code

@cemremengu
Copy link
Contributor Author

@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
Copy link
Owner

sunfuze commented May 13, 2018

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
Copy link
Contributor Author

@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.

@cemremengu cemremengu closed this May 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants