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

feat: give ability to set `connectString` directly #192

Merged
merged 3 commits into from Apr 25, 2019

Conversation

3 participants
@zetxx
Copy link
Contributor

commented Apr 24, 2019

No description provided.

@project-bot project-bot bot added this to In progress in Kanban Apr 24, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 24, 2019

Codecov Report

Merging #192 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #192      +/-   ##
=========================================
- Coverage    3.09%   3.09%   -0.01%     
=========================================
  Files          84      84              
  Lines        2844    2845       +1     
  Branches      424     425       +1     
=========================================
  Hits           88      88              
- Misses       2750    2751       +1     
  Partials        6       6
Impacted Files Coverage Δ
packages/core/dialect/oracle/index.ts 0% <0%> (ø) ⬆️

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 a034d43...3da501d. Read the comment docs.

@mickeypearce

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

Hey @zetxx , thanks for PR.

const connectString = (this.credentials.server && this.credentials.port) ?
`${this.credentials.server}:${this.credentials.port}/${this.credentials.database}` :
this.credentials.database;

Now it is possible, if you ommit the this.credentials.server and this.credentials.port, the connectionString maps directly from this.credentials.database.

I agree, maybe it is not super clear.
Docs: https://github.com/mtxr/vscode-sqltools/blob/master/docs/Connections/OracleDB.md#22-alternative-connection-strings

@mtxr

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2019

@zetxx you should add this configuration to packages/extension/package.json and ConnectionInterface too.

Could you do that please?

@mtxr

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2019

@mickeypearce I believe his suggestion worth. Connection string are very used and we should have a explicit way to do that to avoid confusion. I could add it to other dialects too since now it's working just for oracle.

What do you think?

zetxx added some commits Apr 24, 2019

@mickeypearce

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

I agree, having connectString explicitly is much clearer. Let's do it.

@zetxx

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

do i need to do something more ?

@mtxr

This comment has been minimized.

Copy link
Owner

commented Apr 25, 2019

No, that's on me now.
I'll add the same feature for other dialects and release it.

Thanks @zetxx! Welcome too the community!

@mtxr mtxr merged commit 573442a into mtxr:master Apr 25, 2019

3 of 5 checks passed

codecov/patch 0% of diff hit (target 3.09%)
Details
codecov/project 3.09% (-0.01%) compared to a034d43
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate All good!
Details

Kanban automation moved this from In progress to To be released Apr 25, 2019

mtxr added a commit that referenced this pull request Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.