-
Notifications
You must be signed in to change notification settings - Fork 4
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
refactor: Remove BackgroundRealm for MailCoreRealmAccessor #1414
Conversation
714d070
to
f28857c
Compare
Found 52 unused code occurences Expand
|
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.
As a general comment, now errors are throwed at an upper level. From the code I read they could also be shown to the user which I don't think we want.
Before we used try? realm.write
so the errors were ignored. As a good middle ground, maybe we could catch and send to sentry.
@PhilippeWeidmann Sure I'll match the old pattern. My idea was all (DB write) errors should bubble up as they are critical. Silently fail on a write error is probably not ideal. |
I'm see the idea but I don't think this is good UX, the user doesn't care / doesn't know what is a db write error. If we bubble up errors they have to be correctly translated and have a meaningful message. |
16a7d64
to
0dbac2a
Compare
0dbac2a
to
81699fb
Compare
|
Substitute
BackgroundRealm
forTransactionable
as no longer make sense.Should be merged after #1413