-
Notifications
You must be signed in to change notification settings - Fork 155
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
AsyncAction confirming with param #3667
Conversation
…firmingNoParams` data object. This will allow inheritance of `AsyncAction.Confirming` with parameter(s).
…teData when confirming decline of invite.
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Quality Gate passedIssues Measures |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3667 +/- ##
===========================================
- Coverage 82.81% 82.81% -0.01%
===========================================
Files 1747 1748 +1
Lines 41779 41771 -8
Branches 5107 5106 -1
===========================================
- Hits 34599 34592 -7
Misses 5365 5365
+ Partials 1815 1814 -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.
LGTM, just a comment.
// Note: confirming will always be of type ConfirmingDeclineInvite. | ||
if (confirming is ConfirmingDeclineInvite) { |
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.
Can't we just force cast it then?
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.
We could but why take the risk of a crash?
Previous code was not val invite = state.invite!!
, so I prefer to keep the code safe.
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.
We could but why take the risk of a crash?
Can we then log an error or upload some error report to sentry? I'm worried this might just silently fail, although the tests should cover it.
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.
My previous attempt to fix this would have avoid this drawback. But it was too invasive, so we go with this approach. This is not a new issue, so I guess this is fine.
Content
AsyncAction.ConfirmingNoParams
data object.This will allow inheritance of
AsyncAction.Confirming
with parameter(s).Replacement for #3645 (other change from #3645 will be in separate PRs.)
Can be reviewed commit per commit
Motivation and context
Improve code when confirmation need data to be rendered.
Screenshots / GIFs
Tests
Tested devices
Checklist