-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
Add error boundary wrapper type #1556
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1556 +/- ##
=======================================
- Coverage 75.6% 75.4% -0.1%
=======================================
Files 79 80 +1
Lines 7237 7290 +53
=======================================
+ Hits 5464 5495 +31
- Misses 1773 1795 +22
|
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.
This is very cool! Took me a while to grok this, but it's a very nice non-breaking way to inject an opt-in error in here. Left some suggestions and a few minor questions, but no major complaints.
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.
All good on my end.
207faca
to
c5f6d68
Compare
Fixes kube-rs#774 Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
Signed-off-by: Natalie Klestrup Röijezon <nat@nullable.se>
c5f6d68
to
5a7cefa
Compare
Motivation
Fixes #774. In short, bulk requests (like
Api::list
andwatcher
) are currently unable to cope with failing to parse individual objects, and instead treat the whole list as failed.Solution
This PR introduces a new wrapper type
DeserializeGuard<T>
, which can be used instead ofApi<T>
to opt into a more lenient style, where the list parse succeeds but each object reports errors separately.Since it implements
Resource
, it should also work transparently for APIs building on top ofApi<T>
, such asController<T>
.Ideally, it'd be nice if at least
Controller<T>
would use it transparently, but that would mean a breaking change on theController
types to take the rightApi
variant.