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

support for pg Promises #64

Merged
merged 6 commits into from
Nov 12, 2018
Merged

support for pg Promises #64

merged 6 commits into from
Nov 12, 2018

Conversation

jafl
Copy link
Contributor

@jafl jafl commented Oct 24, 2018

Issue #, if available:

Missing support for pg Promises.

Description of changes:

Restructured code to be parallel with mysql and added logic to handle case when __query() returns a Promise.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jafl jafl changed the title Postgres query support for pg Promises Oct 24, 2018
This was referenced Oct 24, 2018
if (this.queryQueue.length === 0) {
query = this.activeQuery;
} else {
query = this.queryQueue[this.queryQueue.length-1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you put some comments on what this line mean?

@haotianw465
Copy link
Contributor

Looks good to me, posted a minor question.

} else {
args.sql = argsObj[0];
args.values = typeof argsObj[1] !== 'function' ? argsObj[1] : null;
args.callback = !args.values ? argsObj[1] : (typeof argsObj[2] === 'function' ? argsObj[2] : undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the recent fix for mysql also apply here? #66

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, any update on this? Please let me know your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated it in my latest commit: 1474129

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the confusion. My question is that should the change typeof argsObj[1] === 'function' ? on mysql_p.js also be applied to postgres_p.js file here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it should.

@jafl
Copy link
Contributor Author

jafl commented Nov 9, 2018

Do you need anything additional for this to be merged?

@haotianw465 haotianw465 merged commit 2e4fcfa into aws:master Nov 12, 2018
@haotianw465
Copy link
Contributor

Thank you for your contribution. I have merged the pull request. Will prepare for a release as soon as possible.

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.

None yet

2 participants