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

Update to pymongo 3.7 #149

Merged
merged 2 commits into from
Nov 28, 2018
Merged

Update to pymongo 3.7 #149

merged 2 commits into from
Nov 28, 2018

Conversation

lafrech
Copy link
Collaborator

@lafrech lafrech commented Nov 28, 2018

https://api.mongodb.com/python/current/changelog.html

Deprecated pymongo.collection.Collection.count() and pymongo.cursor.Cursor.count(). These two methods use the count command and may or may not be accurate, depending on the options used and connected MongoDB topology. Use count_documents() instead.

This PR is based on #148.

It adds a count method to PyMongoDocument. (I named it count and not count_documents to match umongo's TxMongoDocument. I have no objection to naming it count_documents.)

I'd do the same for MotorAsyncIODocument except it requires motor>=2.0, which is not supported.

Since we already require pymongo 3.x, I figured bumping the requirement wouldn't be too much of an issue and would save us the pain of supporting both 3.7 and 3.6-. I could rework to only add the new count method if using 3.7 and only require 3.7 in the tests. But I'm not sure it is worth the trouble.

@coveralls
Copy link

Coverage Status

Coverage decreased (-9.9%) to 85.776% when pulling 3a37472 on dev_update_to_pymongo_3_7 into db076f6 on master.

@coveralls
Copy link

coveralls commented Nov 28, 2018

Coverage Status

Coverage increased (+0.008%) to 95.704% when pulling 47f05b8 on dev_update_to_pymongo_3_7 into 9bc2f02 on master.

@touilleMan
Copy link
Member

It adds a count method to PyMongoDocument. (I named it count and not count_documents to match umongo's TxMongoDocument. I have no objection to naming it count_documents.)

I'd better keep count_documents if it's the name in the pymongo driver (I guess people are more interested in consistency between umongo and the driver they use than between drivers)
Another possibility would be to provide both name (count and count_documents), as you like.

Since we already require pymongo 3.x, I figured bumping the requirement wouldn't be too much of an issue and would save us the pain of supporting both 3.7 and 3.6

Agree, let's keep things simple ;-)

@touilleMan
Copy link
Member

otherwise seems good to me 👍

@lafrech
Copy link
Collaborator Author

lafrech commented Nov 28, 2018

Alright.

I changed to count_documents. Note that my implementation differs from pymongo's in that the filter parameter is optional and defaults to {}. I can change that if you prefer to adhere more strictly. I don't understand why pymongo didn't make this argument optional. Maybe their good reason would apply here, too.

@lafrech lafrech merged commit 4dde772 into master Nov 28, 2018
@lafrech lafrech deleted the dev_update_to_pymongo_3_7 branch November 28, 2018 13:40
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.

None yet

3 participants