-
Notifications
You must be signed in to change notification settings - Fork 269
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
fix(ffi): don't panic when running into an unknown membership state #4144
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4144 +/- ##
=======================================
Coverage 84.76% 84.76%
=======================================
Files 269 269
Lines 28848 28848
=======================================
+ Hits 24453 24454 +1
+ Misses 4395 4394 -1 ☔ View full report in Codecov by Sentry. |
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 suppose it's better than a panic.
There's no way to expose the private custom variant's content anyways, right? |
It was actually quite simple to handle custom memberships; not sure how the apps will like it? cc @stefanceriu @jmartinesp Re-requesting a review, since there's a non-trivial change in there. |
Turns out you need to name that field but otherwise it shouldn't be a problem for the app |
MembershipState::Leave | ||
Ok(MembershipState::Leave) | ||
} | ||
matrix_sdk::ruma::events::room::member::MembershipState::_Custom(_) => { |
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 think that this is not the intended way, as the _Custom
variant is hidden and instead you're supposed to use the catch all, but then other known states won't be handled (there are none other known states but the enum is non-exhaustive).
I think we can live with that despite the intentions of Ruma.
aa0f9b9
to
56c7f00
Compare
Fixes #1254. This adds a bandaid in the form of converting
Into
impl toTryInto
, and then will happily ignore errors in multiple places; unknown states are logged in warnings, so we can still get some information about it, would they become an issue in the future.