Skip to content

Conversation

juliusgeo
Copy link
Contributor

No description provided.

self.test.assertIsInstance(actual, abc.MutableSequence)

self.test.assertIsInstance(
actual, (abc.MutableSequence, pymongo.command_cursor.CommandCursor)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary because list_indexes returns a CommandCursor, not a MutableSequence like expected. We do not seem to test the type of the output of list_indexes elsewhere in the spec tests, and in the pymongo specific tests we always convert to list immediately.

Copy link
Member

Choose a reason for hiding this comment

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

Let's return a list from _collectionOperation_listIndexes instead. That's what we do for find and listCollections.

@juliusgeo juliusgeo marked this pull request as ready for review June 14, 2022 13:41
name: <string>, // optional, otherwise automatically generated
v: <int>, // optional, must be ‘2’ if provided
}

- `**kwargs` (optional): additional keyword arguments will
be passed as options for the `create collection command`_
Copy link
Member

Choose a reason for hiding this comment

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

Given this description of kwargs, I wonder if it would be more consistent to not add an explicit "clustered_index" parameter and instead document clusteredIndex as an acceptable kwarg below. If so, we probably do the same for encrypted_fields.

We would make the same change to Collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM!

self.test.assertIsInstance(actual, abc.MutableSequence)

self.test.assertIsInstance(
actual, (abc.MutableSequence, pymongo.command_cursor.CommandCursor)
Copy link
Member

Choose a reason for hiding this comment

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

Let's return a list from _collectionOperation_listIndexes instead. That's what we do for find and listCollections.

- `**kwargs` (optional): additional keyword arguments will
be passed as options for the `create collection command`_
Examples of valid
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

There are still a few test failures related to encryptedFields to be resolved.

name: <string>, // optional, otherwise automatically generated
v: <int>, // optional, must be ‘2’ if provided
}

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this section to avoid duplicating the docs. We already document encryptedFields/clusteredIndex and other options in create_collection and instantiating a Collection directly is uncommon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@juliusgeo juliusgeo requested a review from ShaneHarvey June 15, 2022 18:17
@blink1073
Copy link
Member

@juliusgeo you picked up a merge conflict

@juliusgeo juliusgeo requested a review from blink1073 June 15, 2022 18:35
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

@juliusgeo juliusgeo merged commit 02a9df6 into mongodb:master Jun 15, 2022
juliusgeo added a commit to juliusgeo/mongo-python-driver that referenced this pull request Jun 29, 2022
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