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

Updated to be compatible with node-postgres #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nevon
Copy link

@Nevon Nevon commented Jul 26, 2017

I started out just fixing two bugs:

  • It didn't respect the username setting, because it's called username in sequelize's config file, and user in pg
  • It didn't connect to the right database. The default database that is created by postgres is just called postgres, but since you were removing the database from the configuration object, it would try to connect to process.env.USER instead, which most likely doesn't exist. So now we will either connect to process.env.POSTGRES_DB (as defined by postgres) or postgres.

Then I got carried away when I saw a bunch of duplication. What I ended up doing:

  • Replace pg with pg-promise so that we don't need a bunch of callbacks all over the place
  • Extract connection establishment and "user interface" stuff from the create/drop to make it easier to understand what is going on in there.
  • Use parameterized queries where possible to avoid accidents.
  • Use prettier for formatting, and got rid of linting stuff
  • Got rid of all unused test stuff
  • Added yarn lockfile

This repo looks abandoned, so I'll be using my fork, but I thought I'd offer up the changes in case you were interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant