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

Add support for using promises in Cursor methods #2554

Merged
merged 4 commits into from Jul 27, 2021

Conversation

Bluenix2
Copy link
Contributor

Fixes #2545. This change was simply mirrored from Client.query.

I did base this pull request on #2553, expecting that to also be merged (that way no merge conflicts arise). If this is unwanted I could change that.

@Bluenix2 Bluenix2 force-pushed the cursor-promises branch 3 times, most recently from 07759ac to dabeb9b Compare May 30, 2021 16:25
@Bluenix2
Copy link
Contributor Author

Bluenix2 commented May 30, 2021

I did base this pull request on #2553, expecting that to also be merged (that way no merge conflicts arise). If this is unwanted I could change that.

I reverted this in the case I want to add more commits to that pull request. This means that currently this pull request will fail, but I'll rebase this onto master when that (possibly?) gets merged. I'll have to re-add this._Promise then.

@Bluenix2 Bluenix2 changed the title Add support for using promises in Cursor.read() Add support for using promises in Cursor methods Jun 4, 2021
@Bluenix2 Bluenix2 marked this pull request as draft June 21, 2021 22:35
@Bluenix2
Copy link
Contributor Author

Thought I'd correctly mark this as draft until the pull request this currently depends on gets reviewed.

@brianc
Copy link
Owner

brianc commented Jun 22, 2021

Adding promises in here is totally cool...need some tests for the functionality though!

@Bluenix2
Copy link
Contributor Author

Yup I am going to get to that now!

@brianc
Copy link
Owner

brianc commented Jun 22, 2021 via email

@Bluenix2 Bluenix2 force-pushed the cursor-promises branch 2 times, most recently from 73470a5 to 66a9434 Compare June 22, 2021 19:07
@Bluenix2
Copy link
Contributor Author

When writing tests, prettier tries to enforce the following style:

  it('fetch 6 when asking for 10', function (done) {
    const cursor = this.pgCursor(text)
    cursor
      .read(10)
      .then((res) => assert.strictEqual(res.length, 6))
      .error((err) => assert.ifError(err))
  })

When I'd expect the following instead:

  it('fetch 6 when asking for 10', function (done) {
    const cursor = this.pgCursor(text)
    cursor.read(10)
      .then((res) => assert.strictEqual(res.length, 6))
      .error((err) => assert.ifError(err))
  })

Thoughts on this?

@Bluenix2
Copy link
Contributor Author

There should be tests for this now; I thought testing resolving, rejecting and multiple read() calls was sufficient.

Removes usage of `done()` since we can end the test when we exit the function

Co-Authored-By: Charmander <~@charmander.me>
@Bluenix2 Bluenix2 marked this pull request as ready for review June 23, 2021 16:36
@brianc
Copy link
Owner

brianc commented Jun 23, 2021

When writing tests, prettier tries to enforce the following style:

Thoughts on this?

I always just let prettier do the formatting. Easier to not even have to care at all & everything looks consistent.

Copy link
Owner

@brianc brianc 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! looks good to me.

nit: you could use let instead of var.

@Bluenix2
Copy link
Contributor Author

Thanks for this! looks good to me.

nit: you could use let instead of var.

I wasn't sure if it would cause issues with scoping since there's a bit of a difference, but that should be done now @brianc

@Bluenix2
Copy link
Contributor Author

Bluenix2 commented Jul 9, 2021

@brianc Is there anything hindering this from getting merged?

@brianc
Copy link
Owner

brianc commented Jul 27, 2021

This is great really sorry for the delay - life is crazy 💻 🐱 I'll get a semver minor out today w/ the changes. Thank you so much! ❤️

@brianc brianc merged commit aedaa59 into brianc:master Jul 27, 2021
@Bluenix2 Bluenix2 deleted the cursor-promises branch July 27, 2021 15:50
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.

Why does pg.Cursor still use callbacks?
3 participants