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

✨ Add new SyncEngine, support async and sync code - with fixed session #231

Closed
wants to merge 33 commits into from

Conversation

tiangolo
Copy link
Collaborator

✨ Add new SyncEngine, support async and sync code - with fixed session

This is the same as #225, but it includes the fixes to share the same session in #227

This would supersede #225 if the session fix in #227 is accepted.

I'm doing it here as an additional PR because there are a couple of changes and fixes needed in the new SyncEngine to handle that same issue with the session.

@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #231 (8850bae) into master (623fa27) will decrease coverage by 0.05%.
The diff coverage is 99.79%.

❗ Current head 8850bae differs from pull request most recent head 51d2585. Consider uploading reports for the commit 51d2585 to get more accurate results

@@             Coverage Diff             @@
##            master     #231      +/-   ##
===========================================
- Coverage   100.00%   99.94%   -0.06%     
===========================================
  Files           38       38              
  Lines         2725     3424     +699     
  Branches       413      483      +70     
===========================================
+ Hits          2725     3422     +697     
- Misses           0        1       +1     
- Partials         0        1       +1     
Flag Coverage Δ
tests-3.6-4-standalone 98.29% <95.12%> (-1.42%) ⬇️
tests-3.6-4.2-standalone 98.29% <95.12%> (-1.42%) ⬇️
tests-3.6-4.4-standalone 98.29% <95.12%> (-1.42%) ⬇️
tests-3.7-4-standalone 98.14% <95.12%> (-1.38%) ⬇️
tests-3.7-4.2-standalone 98.14% <95.12%> (-1.38%) ⬇️
tests-3.7-4.4-standalone 98.14% <95.12%> (-1.38%) ⬇️
tests-3.8-4-replicaSet 98.30% <99.79%> (+0.36%) ⬆️
tests-3.8-4-standalone 98.10% <95.14%> (-1.35%) ⬇️
tests-3.8-4.2-sharded 96.90% <95.14%> (-1.05%) ⬇️
tests-3.8-4.2-standalone 98.10% <95.14%> (-1.35%) ⬇️
tests-3.8-4.4-standalone 98.10% <95.14%> (-1.35%) ⬇️
tests-3.9-4-standalone 97.98% <94.93%> (-1.32%) ⬇️
tests-3.9-4.2-standalone 97.98% <94.93%> (-1.32%) ⬇️
tests-3.9-4.4-standalone 97.98% <94.93%> (-1.32%) ⬇️

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

Impacted Files Coverage Δ
odmantic/engine.py 99.20% <98.57%> (-0.80%) ⬇️
odmantic/__init__.py 100.00% <100.00%> (ø)
tests/integration/fastapi/test_doc_example.py 100.00% <100.00%> (ø)
tests/integration/test_embedded_model.py 100.00% <100.00%> (ø)
tests/integration/test_engine.py 100.00% <100.00%> (ø)
tests/integration/test_engine_reference.py 100.00% <100.00%> (ø)
tests/integration/test_query.py 100.00% <100.00%> (ø)
tests/integration/test_types.py 100.00% <100.00%> (ø)
tests/integration/test_zoo.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@tiangolo
Copy link
Collaborator Author

I added a couple of small tests to check the errors when motor is not installed, this should fix the line missing coverage. 😎

@art049
Copy link
Owner

art049 commented Aug 19, 2022

Awesome @tiangolo !
I just reviewed everything and it looks perfect to me.

There is just a tiny rebase required and it will be good for a merge!
I can handle the documentation part if you want, it shouldn't be that long.

Can't wait to see what you plan to do with bulk_writes and switch_collection!

@art049
Copy link
Owner

art049 commented Aug 21, 2022

Actually I though about it again and it might be quite painful for existing users to have to change the dependency in order to upgrade to the new release.
I think it would be smoother to let the motor dependency as required.
I'm not totally sure but wdyt @tiangolo ?

@art049
Copy link
Owner

art049 commented Aug 24, 2022

Merged in #242 !
Many thanks @tiangolo ! 🔥

@art049 art049 closed this Aug 24, 2022
@tiangolo
Copy link
Collaborator Author

Awesome, thanks a lot! 🚀

About having Motor as optional or not, I think that's totally up to you. I get it that as ODMantic was always Motor-specific it could feel weird suddenly having that as optional.

If it was a new project, I guess it would be a no-brainer but I get that it can make sense to keep it required for backwards compatibility.

Maybe it can be done in a future release after having some warning in the docs for a while or something like that. Anyway, I don't feel strongly about it in one way or the other. 😅

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.

2 participants