-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(NODE-4079): estimated document count uses count #3244
Conversation
Note that this commit for DRIVERS-2228 (mongodb/specifications@021cbc8) did not include all the needed test changes to get the additional tests passing. Those were part of mongodb/specifications@c430376 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec changes say we need to document the behavior (in addition to the release notes). Should we modify the comment on Collection.estimatedDocumentCount
to include the relevant information?
Can we also create a jira ticket and mark it for the next minor (4.7.0) with these changes, so we remember to include the relevant information in the release notes? See https://mongodb.slack.com/archives/GGWBN4ZNK/p1652725071464819 |
|
I've updated the TSDoc to note this and link to the MongoDB docs. |
There's also this blurb from the spec:
Should we mention this in our comment as well? |
Put in TSDoc as well now. |
@@ -535,81 +535,81 @@ describe('Collection', function () { | |||
}); | |||
}); | |||
|
|||
describe('(countDocuments)', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests pass locally but fail on CI so I just rewrote them. They also weren't testing anything in some cases since any type of count would return 0
if no documents were inserted.
Description
Updates
estimatedDocumentCount
to use thecount
command.What is changing?
Changes the operation to use a
count
command instead of anaggregate
.Is there new documentation needed for these changes?
None
What is the motivation for this change?
NODE-4079
Double check the following
npm run check:lint
script<type>(NODE-xxxx)<!>: <description>