-
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: replace actix_web::web::Data
by Arc
in models
#7418
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #7418 +/- ##
=============================================
+ Coverage 29.15% 54.84% +25.69%
=============================================
Files 1164 67 -1097
Lines 144102 3878 -140224
Branches 2811 0 -2811
=============================================
- Hits 42013 2127 -39886
+ Misses 100428 1751 -98677
+ Partials 1661 0 -1661
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5b10688
to
c363926
Compare
actix_web::web::Data
from modelsactix_web::web::Data
by Arc
in models
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 cleanup that's long overdue! :D
I'd replace the occurrences of db_pool.clone().into_inner()
by db_pool.into_inner()
directly when possible. When it's not, I'd prefer to extract the Arc
first and then clone the arc directly.
That doesn't block this PR, but I'd rather get rid of the Data
as early as possible.
foo(db_pool.clone().into_inner());
bar(db_pool.into_inner());
// would become
let db_pool = db_pool.into_inner();
foo(db_pool.clone());
bar(db_pool);
(You can use sed -y
to swap clone()
and into_inner()
to quickly spot the places that need that change.)
This will likely conflict with #7281
c363926
to
e89eac8
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, thanks! :)
e89eac8
to
c354aed
Compare
Part of #6980