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

fix(NODE-6284): make sparsity and trimFactor optional #4189

Merged
merged 12 commits into from
Aug 20, 2024

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Aug 5, 2024

Description

What is changing?

Sparsity and trimFactor, two options for the QE Range, have been marked optional. All range v2 spec tests are syncced. CSFLE prose test 23 has been implemented.

Additionally, the crypto callbacks tests have been removed. The bindings now automatically load crypto callbacks from C++ with all compatible mongodb-client-encryption versions. This means we can no longer spy on or sinon stub the cypto callbacks, so these tests are broken.

Is there new documentation needed for these changes?

What is the motivation for this change?

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

@baileympearson baileympearson marked this pull request as ready for review August 14, 2024 21:35
@baileympearson baileympearson changed the base branch from NODE-6284 to main August 14, 2024 21:38
@evergreen-ci-prod evergreen-ci-prod bot mentioned this pull request Aug 14, 2024
5 tasks
@W-A-James W-A-James self-assigned this Aug 15, 2024
@W-A-James W-A-James self-requested a review August 15, 2024 14:43
@W-A-James W-A-James added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Aug 15, 2024
W-A-James
W-A-James previously approved these changes Aug 16, 2024
@W-A-James W-A-James added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Aug 16, 2024
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.

We're putting the release highlights of this change into: #4141 correct? Could you fill those out with the changes we're making here?

@baileympearson
Copy link
Contributor Author

We're putting the release highlights of this change into: #4141 correct? Could you fill those out with the changes we're making here?

This is a fix to changes made in another PR that hasn't been released. There are no release notes for this - as far as users are concerned, this work never happened.

@nbbeeken
Copy link
Contributor

This is a fix

Maybe I am overlooking something in this PR. Is it not the case that trimFactor?: Int32; is a new line of code? I do not see trimFactor in src on main.

I figure that we can still call this a "fix" PR but that this "feature" is a logical part of #4141 in the sense that it is "adding support for range v2"

@baileympearson
Copy link
Contributor Author

This is a fix

Maybe I am overlooking something in this PR. Is it not the case that trimFactor?: Int32; is a new line of code? I do not see trimFactor in src on main.

I figure that we can still call this a "fix" PR but that this "feature" is a logical part of #4141 in the sense that it is "adding support for range v2"

I guess that was an oversight. The full feature will be documented in #4190 when the time comes.

@nbbeeken
Copy link
Contributor

FYI, there's a lint error in the test file.

nbbeeken
nbbeeken previously approved these changes Aug 19, 2024
@W-A-James W-A-James merged commit 8622545 into mongodb:main Aug 20, 2024
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants