Skip to content

Comments

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

Closed
tiangolo wants to merge 33 commits intoart049:masterfrom
tiangolo:sync-engine-fixed-session
Closed

✨ Add new SyncEngine, support async and sync code - with fixed session#231
tiangolo wants to merge 33 commits intoart049:masterfrom
tiangolo:sync-engine-fixed-session

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.

tiangolo added 30 commits June 8, 2022 15:09
@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. 😅

@tiangolo tiangolo deleted the sync-engine-fixed-session branch June 11, 2025 21:09
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