-
-
Notifications
You must be signed in to change notification settings - Fork 394
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
feat!: static handler configuration #3900
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.0 #3900 +/- ##
==========================================
- Coverage 98.30% 98.26% -0.05%
==========================================
Files 342 342
Lines 15370 15350 -20
Branches 1694 1662 -32
==========================================
- Hits 15109 15083 -26
- Misses 129 133 +4
- Partials 132 134 +2 ☔ View full report in Codecov by Sentry. |
0d74792
to
c2fc97d
Compare
c2fc97d
to
4f2e51f
Compare
Co-authored-by: Cody Fincher <[email protected]>
Co-authored-by: euri10 <[email protected]>
81fbfe6
to
f373a3d
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.
I don't have much to add other than what we've already discussed here and on discord. Great work on this.
Quality Gate failedFailed conditions See analysis details on SonarQube Cloud Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE |
Basic idea is this: Instead of mutating handlers in place, treat them as static configuration objects. Once the handler tree has been collected, reduce all the layers into a single handler by recursively merging them.
Why? The idea is that this design is much less error prone and easier to follow (e.g. a lot of things currently need to happen in a specific order, otherwise they'll break). It also supports a pattern we're using in a few places now (websocket listeners + stream), where you essentially want to replace the handler with a new one. Another benefit is that this can get rid of all the deep copying we're currently doing :)
Goals
Router
toLitestar
instanceresolve_
methodsdeepcopy
ing of route handlers during registrationExtract common handler config into config objects (dataclasses), so they're easier to merge / manipulateMake route handlers purely static by removing all in-place mutationI've decided to strike the last two goals, as it would make a migration path forward needlessly complex. With the removal of the now deprecated functionality in 4.0, this will be a lot easier to achieve, and we're still in a much better place with this than before.