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

refactor: replace symbol properties in cursor classes #4133

Merged
merged 8 commits into from
Jun 7, 2024

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Jun 5, 2024

Description

What is changing?

  • AbstractCursor
    • Removed all symbol properties in favor of equivalently named strings.
    • Changed "options" to cursorOptions, no need for the getter indirection
    • Added is in front of booleans that have getters
  • FindCursor
    • kFilter -> cursorFilter since there is a "filter" method
    • builtOptions -> findOptions, I think the naming came from TS translation days, it is true that the options are built from all the helpers (if one uses those) but in the end they are options for find
  • AggregationCursor
    • pipline is a public readonly property which is equivalent to the getter that fetched the symbol property before
  • ChangeStreamCursor
    • No symbol properties here, but the "options" property shared the same name as AbstractCursor's "options" until I changed it to "cursorOptions".
    • But that led me to take a look at the class properties, we never set resumeAfter/startAfter on the class, thos remain on the options, unless I missed something.

patch

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

Symbol properties served as "private" before we had typescript. We prefer using plain string properties now for better DX, and testing internals.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken
Copy link
Contributor Author

nbbeeken commented Jun 5, 2024

evergreen patch

W-A-James
W-A-James previously approved these changes Jun 5, 2024
@W-A-James W-A-James dismissed their stale review June 5, 2024 20:03

test failures

@nbbeeken nbbeeken force-pushed the refactor-remove-sym-cursor branch 2 times, most recently from 001cdd9 to ce31f22 Compare June 7, 2024 18:40
Base automatically changed from refactor-cursor-nextBatch to main June 7, 2024 18:45
@nbbeeken nbbeeken force-pushed the refactor-remove-sym-cursor branch from ce31f22 to 0d7c85e Compare June 7, 2024 18:49
@nbbeeken nbbeeken requested a review from W-A-James June 7, 2024 18:50
@W-A-James W-A-James merged commit 28c7526 into main Jun 7, 2024
22 of 29 checks passed
@W-A-James W-A-James deleted the refactor-remove-sym-cursor branch June 7, 2024 19:38
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.

2 participants