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: introduce AbstractCursor and its concrete subclasses #2619

Merged
merged 14 commits into from
Nov 25, 2020

Conversation

mbroadst
Copy link
Member

This change introduces a fundamental redesign of the cursor types in the driver. The first change is to add a new AbstractCursor
type, which is only concerned with iterating a cursor (using getMore) once it has been initialized. The _initialize method must be implemented by subclasses. The concrete subclasses are generally builders for find and aggregate commands, each providing their own custom initialization method.

NODE-2809

@mbroadst mbroadst force-pushed the NODE-2809/combine-cursors branch 2 times, most recently from 4d30352 to df8848e Compare November 12, 2020 15:10
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

👏 hats off 🎩 to this incredible chunk of cursor work!!

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/cursor/abstract_cursor.ts Show resolved Hide resolved
src/cursor/abstract_cursor.ts Outdated Show resolved Hide resolved
src/cursor/abstract_cursor.ts Outdated Show resolved Hide resolved
src/cursor/abstract_cursor.ts Show resolved Hide resolved
src/cmap/wire_protocol/query.ts Show resolved Hide resolved
src/change_stream.ts Outdated Show resolved Hide resolved
src/cursor/find_cursor.ts Outdated Show resolved Hide resolved
src/cursor/find_cursor.ts Show resolved Hide resolved
@mbroadst mbroadst force-pushed the NODE-2809/combine-cursors branch 2 times, most recently from 6dac300 to 631f982 Compare November 13, 2020 13:19
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

A few questions/nits but this looks great. I reviewed the src changes closely but only took a surface level look at the test changes; I'll take a closer look at tests in my follow-up review.

src/cursor/abstract_cursor.ts Outdated Show resolved Hide resolved
src/cursor/abstract_cursor.ts Show resolved Hide resolved
src/cursor/abstract_cursor.ts Show resolved Hide resolved
src/cursor/abstract_cursor.ts Show resolved Hide resolved
test/functional/operation_example.test.js Show resolved Hide resolved
src/cursor/abstract_cursor.ts Show resolved Hide resolved
@mbroadst mbroadst requested a review from emadum November 18, 2020 14:03
@mbroadst mbroadst force-pushed the NODE-2809/combine-cursors branch 2 times, most recently from 673e28d to b7a4f1a Compare November 20, 2020 22:00
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

LGTM! 👏

test/functional/abstract_cursor.test.js Outdated Show resolved Hide resolved
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

lgtm! (pending a rebase 👍 )

This change introduces a fundamental redesign of the cursor types
in the driver. The first change is to add a new `AbstractCursor`
type, which is only concerned with iterating a cursor (using
`getMore`) once it has been initialized. The `_initialize` method
must be implemented by subclasses. The concrete subclasses are
generally builders for `find` and `aggregate` commands, each
providing their own custom initialization method.

NODE-2809
@mbroadst mbroadst merged commit a2d78b2 into master Nov 25, 2020
@mbroadst mbroadst deleted the NODE-2809/combine-cursors branch November 25, 2020 13:06
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.

3 participants