Implement `remove` with tests and doc example

[joeriddles]

Closes Issue #121

Notes

• I read the contribution guidelines.
• I created DocumentsNotFoundError to aggregate multiple documents not being found.
• I don't believe delete_many should typically raise DocumentsNotFoundError, unless another thread deletes one of the instances returned from find before it is deleted from within delete_many. This should be a rare scenario typically.

[joeriddles]

@art049 please review at your earliest convenience.

[joeriddles]

+ @ChronoDK

[codecov[bot]]

Codecov Report

Merging #147 (d16edfd) into master (82ba3d8) will increase coverage by 0.39%. The diff coverage is n/a.

:exclamation: Current head d16edfd differs from pull request most recent head 1bf579a. Consider uploading reports for the commit 1bf579a to get more accurate results

@@             Coverage Diff             @@
##           master      #147      +/-   ##
===========================================
+ Coverage   99.60%   100.00%   +0.39%     
===========================================
  Files          38        38              
  Lines        2775      2723      -52     
  Branches      495       178     -317     
===========================================
- Hits         2764      2723      -41     
+ Misses          8         0       -8     
+ Partials        3         0       -3     

Flag	Coverage Δ
tests-3.10-4-standalone	?
tests-3.10-4.2-standalone	?
tests-3.10-4.4-standalone	?
tests-3.6-4-standalone	99.70% <0.00%> (?)
tests-3.6-4.2-standalone	99.70% <0.00%> (?)
tests-3.6-4.4-standalone	99.70% <0.00%> (?)
tests-3.7-4-standalone	99.51% <0.00%> (+0.86%)	:arrow_up:
tests-3.7-4.2-standalone	99.51% <0.00%> (+0.86%)	:arrow_up:
tests-3.7-4.4-standalone	99.51% <0.00%> (+0.86%)	:arrow_up:
tests-3.8-4-replicaSet	97.94% <0.00%> (-0.04%)	:arrow_down:
tests-3.8-4-standalone	99.44% <0.00%> (+0.85%)	:arrow_up:
tests-3.8-4.2-sharded	97.94% <0.00%> (+0.82%)	:arrow_up:
tests-3.8-4.2-standalone	99.44% <0.00%> (+0.85%)	:arrow_up:
tests-3.8-4.4-standalone	99.44% <0.00%> (+0.85%)	:arrow_up:
tests-3.9-4-standalone	99.33% <0.00%> (+0.88%)	:arrow_up:
tests-3.9-4.2-standalone	99.33% <0.00%> (+0.88%)	:arrow_up:
tests-3.9-4.4-standalone	99.33% <0.00%> (+0.88%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
odmantic/config.py	100.00% <0.00%> (ø)
odmantic/engine.py	100.00% <0.00%> (ø)
tests/unit/test_field.py	100.00% <0.00%> (ø)
tests/unit/test_reference.py	100.00% <0.00%> (ø)
tests/integration/test_engine.py	100.00% <0.00%> (ø)
tests/unit/test_model_definition.py	100.00% <0.00%> (ø)
tests/unit/test_json_serialization.py	100.00% <0.00%> (ø)
tests/integration/test_engine_reference.py	100.00% <0.00%> (ø)
odmantic/model.py	100.00% <0.00%> (+2.47%)	:arrow_up:
odmantic/typing.py	100.00% <0.00%> (+25.00%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[art049]

Hey @joeriddles thanks for the PR ! I unlocked the CI and it seems an issue was created with the new mkdocs release. I fixed it in https://github.com/art049/odmantic/commit/d16edfdb7cb93ce0c9da0473c39c5fbf10727c03 (on master). If you rebase, it should fix it :)

I had a look to the code and it looks good ;) ! One missing element would be to add the example you wrote (docs/examples_src/engine/delete_many.py) in the engine documentation page. On this topic, the automation for the output generation is not really there yet. You'll have to add the comments with the output manually.

The last thing is about the naming of this method. With the current API it might look confusing as we will have:

• engine.delete that remove the instance directly passed to it
• engine.delete_many that takes a query and remove the results of the query

One possibility would be to rename the new method to remove (mongo method with the same name). It could be possible to add a just_one parameter as well to really mimic Mongo's API. With this, delete would be to delete instance we already have and remove would take a filter to perform the deletion. This difference would make sure users can see the difference between the two and the "dangers" of the new method.

If you have other ideas, don't hesitate. As well @adriencaccia ideas are welcomed :)

[Kromtar]

It would be great if you can incorporate this :)

[joeriddles]

Hey @art049 thanks for the reply and review!

One missing element would be to add the example you wrote (docs/examples_src/engine/delete_many.py) in the engine documentation page.

Will do 👍

The last thing is about the naming of this method. With the current API it might look confusing as we will have:

* `engine.delete` that remove the instance directly passed to it

* `engine.delete_many` that takes a query and remove the results of the query

One possibility would be to rename the new method to remove (mongo method with the same name). It could be possible to add a just_one parameter as well to really mimic Mongo's API. With this, delete would be to delete instance we already have and remove would take a filter to perform the deletion. This difference would make sure users can see the difference between the two and the "dangers" of the new method.

I think that's a great call out. I will update the PR accordingly.

---

It would be great if you can incorporate this :)

@Kromtar thanks for the reminder to wrap this PR up! I'll work on it tomorrow.

[joeriddles]

@art049 Please review the updated PR when you can.

[art049]

Closing in favor of #237. Implementing remove directly with collection.deleteMany and collection.delete.