Skip to content
This repository has been archived by the owner on Mar 15, 2021. It is now read-only.

Update config.js #102

Merged
merged 3 commits into from
Aug 4, 2020
Merged

Update config.js #102

merged 3 commits into from
Aug 4, 2020

Conversation

vitaly-t
Copy link
Contributor

@vitaly-t vitaly-t commented Aug 3, 2020

Correcting use of the connection string:

  • Specify defaults for each property separately, with the constructor
  • use Virtual Properties hostname and port when using only the first host, for safer access
  • Database name within connection string is always the first segment of the url path, and not a url parameter

vitaly-t and others added 2 commits August 3, 2020 20:37
Correcting use of the connection string:

* Specify defaults for each property separately, with the constructor
* use Virtual Properties `hostname` and `port` when using only the first host, for safer access
* Database within connection string is always the first segment, within `path`, and not a url parameter
Copy link
Member

@MasterOdin MasterOdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, as it definitely improves the usage here, and gets closer to phasing out the need for setting these variables at all and just relying on the defaults.

Looks like the tests are failing as it cannot find the database, which I'm guessing is blank as I think the default for database here is wrong maybe, or that just need to update https://github.com/sqlectron/sqlectron-core/blob/master/.github/workflows/test.yml to use the proper DSN form.

spec/databases/config.js Outdated Show resolved Hide resolved
Replacing default for `database` with correct `path`.
@vitaly-t
Copy link
Contributor Author

vitaly-t commented Aug 4, 2020

Also, for most frameworks, you do not need to specify default empty password, you just skip it, so you can delete password: '' from the defaults.

@MasterOdin
Copy link
Member

Yup, though the passwords being blank is more a current symptom of the CI and local environments using different passwords and me wanting to unify that first, and then set an actual password. Having it blank was more a reminder to me of just getting around to doing that at some point.

Thanks again though for improving this!

@MasterOdin MasterOdin merged commit c02bccf into sqlectron:master Aug 4, 2020
@vitaly-t vitaly-t deleted the patch-1 branch August 4, 2020 21:15
@MasterOdin MasterOdin mentioned this pull request Aug 16, 2020
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.

2 participants