Add count_documents support to motor asyncio Documents

[wfscheper]

Update API to support v2.0.0 of motor. This involves changing tests
that use the deprecated count() cursor method to use either
count_documents() or to_list() instead, and removing the callback
keyword argument from the to_list wrapper.

[coveralls]

Coverage increased (+0.02%) to 96.117% when pulling 949407a on wfscheper:motor-2.0 into abf294b on Scille:master.

[lafrech]

I'm not familiar with motor and the async part of umongo, but I gave it a look and it looks like it would be easy to support both motor 1.x and motor 2.x with the same umongo codebase.

Dropping currently supported versions of motor would be a breaking change, but supporting new versions while keeping support for older ones would just be a minor change. We can drop motor 1.x in a later major version.

Do you think you could change your PR to keep compatibility with old motor versions?

Basically, that means

• use conditions when the code depends on the motor version (most of it would be in the tests)
• modify setup.py to read ['motor>=1.1,<3.0']
• modify tox.ini to define motor1 and motor2 instead of just motor
• modify .travis.yml to test motor1 and motor2 (maybe we can test motor1 on python3.5 and motor2 on python 3.7 rather than all combinations, to limit CI load/time)

Thanks.

[wfscheper]

@lafrech, you're correct about it being easy to support both versions of motor, so I'm updating my pull request.

I can see two ways to address this. One is that I don't change the public API of umongo and just make the changes you suggested. The other way is to add a new Document classmethod count(filter, *, with_limit_and_skip=False, **kwargs) that does the version handling for the consumer. What are your thoughts?

[wfscheper]

Updated this branch to fix the tests. I also pushed a count-method branch that adds a new method to abstract the different ways of counting documents in motor.

[lafrech]

I'm sorry, @wfscheper, I don't have time for this and I don't use motor. Maybe @touilleMan will be able to check and merge this PR.

I worked on something similar for the pymongo driver (#149) and we chose to stick to the pymongo API by naming the method count_document rather than count. I guess we should do the same here.

[wfscheper]

@lafrech, @touilleMan

What needs to be done to get this merged? I'd really like to use the latest supported version of motor with umongo.

[touilleMan]

@wfscheper I no longer have time to support this project and @lafrech is not a motor user so it seems pretty hard for us to merge this PR
What we can do is to add you to the maintainers of the project (so with commit/release rights) so you'd be able to handle the support of motor (so motor2, and deciding whether or not supporting motor1 is still useful)

What do you think ? ;-)

[wfscheper]

I'm willing to take on maintaining the motor support for umongo. I have two projects at work that are using umongo + motor, so I'll definitely have the motivation.

I will go ahead and update this PR with the changes I made to the count-method branch (renaming the method to count_documents per @lafrech's comment.

[lafrech]

Great!

[touilleMan]

@wfscheper I've just added you to the project with write rights, welcome aboard ;-)

[wfscheper]

Thanks guys! Looking forward to working on this project with you.

[wfscheper]

So while trying to get test coverage over the code I added to WrappedCursor.to_list, I discovered that in motor 1.x passing a callback to to_list raises NotImplementedError: Motor with asyncio prohibits callbacks. I managed to get the callback working and tested for motor 2, but should I raise that same exception instead?

[lafrech]

I'm not sure I understand the point.

Does this mean that Motor 1 and 2 behave the same about callbacks and you could remove the if MOTOR_VERSION < (2, 0) condition in to_list to always use the wrapper? Or that you should remove the whole callback wrapper for both cases?

AFAIU, to_list was broken because passing a callback would fail. In this case, adding a wrapper to allow a callback to be passed is a feature, unrelated to the Motor 1 -> 2 change. You can and should do it for both Motor 1 and 2.

Maybe for clarity, you could do the Motor 1&2 compatibility change in one move, then add the wrapper (for bother Motor versions) in another move if needed.

Just guessing. I have no idea what I'm talking about. As written above I don't use and don't know async frameworks.

[wfscheper]

So the bottom line is that Motor never supported using callbacks with to_list() when using the asyncio version of the driver. If I were more familiar with motor prior to 2.x, I probably would've known that ahead of time.

I feel that it's a bad idea to add support for callbacks to WrappedCursor.to_list when the underlying driver never supported them, as it would cause umongo+motor to behave differently than how motor is documented.

I'm leaning towards removing the callback wrapper entirely and letting motor 1.x throw NotImplementedError and 2.x throw TypeError for the call signature. Seems the best mix of backwards compatibility in umongo and behavioral compatibility with the driver.

[lafrech]

Makes sense to me. Thanks for investigating.

[lafrech]

Great. So this is a non-breaking change and it can be released as a minor version, right?

I just updated AUTHORS.rst.

@wfscheper, would you like to update CHANGELOG and release?

Release process: #84 (comment) or #101 (comment).

(We could add a RELEASE doc like in marshmallow: https://github.com/marshmallow-code/marshmallow/blob/dev/RELEASING.md.)

[wfscheper]

Will do. I'll add a RELEASING.md; its good to have a checklist.