-
Notifications
You must be signed in to change notification settings - Fork 44
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
Better encapsulate diesel's connection to ease the split of editoast_models #8712
Conversation
4e04a73
to
7aa1016
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #8712 +/- ##
============================================
+ Coverage 36.98% 37.01% +0.03%
Complexity 2208 2208
============================================
Files 1255 1255
Lines 113843 113901 +58
Branches 3188 3188
============================================
+ Hits 42103 42164 +61
+ Misses 69845 69842 -3
Partials 1895 1895
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7aa1016
to
5d72b07
Compare
2dccbce
to
bd34c1a
Compare
bd34c1a
to
c93f95e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big work, will be much better. Thank you 🎉
I request a change on the documentation of the re-implementation of transaction
since it's a central but non-trivial function of this new DbConnection
. I believe it's important to give as much context as possible to ease future maintenance.
c93f95e
to
b156a58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge work! I have a few comments about the API, especially for errors.
That's work for another PR since this one is already massive:
- Most functions don't require a
conn: &mut DbConnection
but rather just aconn: DbConnection
which became cheap to clone. - Maybe we could implement
Future
forDbConnection
so thatconn.await
<=>conn.write().await.deref_mut()
- Some sync test util functions might declutter the boilerplate the
RwLock
introduced.
be0e9b3
to
7fcf853
Compare
Co-authored-by: Jean SIMARD <[email protected]> Co-authored-by: Léo VALAIS <[email protected]>
7fcf853
to
6b27efa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving in order not to block the PR, but since I made the modifications myself, this doesn't account for much 🤣
Create an abstraction of the diesel database connection and transactions
part of the #6980
How to test this PR: