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!: remove deprecated Collection.group helper #2609

Merged
merged 9 commits into from
Nov 10, 2020

Conversation

emadum
Copy link
Contributor

@emadum emadum commented Nov 3, 2020

Description

The group command was deprecated in MongoDB 3.4 and removed in
MongoDB 3.6, this PR removes it from the driver.

NODE-1487

What changed?

Are there any files to ignore?

@mbroadst
Copy link
Member

mbroadst commented Nov 5, 2020

@emadum did you mean to request reviewers on this?

@emadum
Copy link
Contributor Author

emadum commented Nov 5, 2020

@emadum did you mean to request reviewers on this?

Oops meant to make this a draft, not quite ready yet.

@emadum emadum marked this pull request as draft November 5, 2020 15:03
@@ -386,7 +330,7 @@ describe('MapReduce', function () {
}
});

it('shouldCorrectlyReturnNestedKeys', {
it.skip('shouldCorrectlyReturnNestedKeys', {
Copy link
Contributor Author

@emadum emadum Nov 6, 2020

Choose a reason for hiding this comment

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

@mbroadst Do you think this test should be rewritten without collection.group, or is it fine to just remove it outright?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to remove outright. It kind of looks like this test is testing the behavior of MongoDB, rather than some behavior of the driver itself

@emadum emadum marked this pull request as ready for review November 6, 2020 18:02

execute(server: Server, callback: Callback<Document>): void {
const cmd: Document = {
group: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is small but: I think this file is the only place where we ever define cmd.group. If so, could we delete any leftover references to cmd.group? I remember seeing it referenced in at least one place (commandSupportsReadConcern) but there may be others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I did some grepping and I think that was the last remaining reference to cmd.group.

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.

🎉

@emadum emadum merged commit cf5c865 into master Nov 10, 2020
@emadum emadum deleted the NODE-1487/remove-group-helpers branch November 10, 2020 16:47
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.

4 participants