-
Notifications
You must be signed in to change notification settings - Fork 45
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
editoast: create editoast_models and move model v2 database tools #7864
Conversation
abb5a0a
to
b1e70bc
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 #7864 +/- ##
============================================
- Coverage 28.45% 28.41% -0.04%
Complexity 2059 2059
============================================
Files 1256 1264 +8
Lines 154603 155097 +494
Branches 3049 3053 +4
============================================
+ Hits 43989 44078 +89
- Misses 108797 109198 +401
- Partials 1817 1821 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
f304a47
to
e7046be
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.
LGTM
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.
Yes, I did it, I reviewed it all 🎉 Thank you for this refactorization. Don't forget the license.
c70033b
to
9b12628
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.
Thanks for this big refactoring 😄
(I don't believe I've checked this many "viewed" since the switch to diesel_async 😅)
I'll summarize here what we discussed yesterday so that we're all on the same page.
On errors
- Although the crate
editoast_models
now exists, we still can't move models into it as they all rely onInternalError
. - The first step would be to redesign the failures of
ModelV2
's API. - Then we'd have to figure out a way to forward the information of the errors defined in sub-crates up to
editoast_views
without manually reading all fields of all variants. - Only then will we be able to start moving models around.
On diesel*
dependencies
The goal is to remove the diesel
dependencies in all crates but editoast_models
.
- We need to finalize the pool V2 and tests migration (which depends on dropping TSV1 ofc 🙃)
- The
DbConnection
type must be made opaque. - All db queries in the views must be moved into the most relevant model.
That's all I believe?
19b6b2f
to
ff82bba
Compare
ff82bba
to
4c8a295
Compare
ca951fa
to
54d6a65
Compare
54d6a65
to
e14e6f5
Compare
No description provided.