-
Notifications
You must be signed in to change notification settings - Fork 33
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
Refactoring ACI/PNI storage types #275
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #275 +/- ##
==========================================
- Coverage 17.21% 17.07% -0.15%
==========================================
Files 38 38
Lines 3073 3099 +26
==========================================
Hits 529 529
- Misses 2544 2570 +26 ☔ 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.
Let's ship it, I think I had a bit more quality of life improvements to add, but I can do it later! Thanks for this 😄
FYI, this is 100% untested. I probably want to ship some more things, but https://gitlab.com/whisperfish/whisperfish/-/merge_requests/530 is on hold currently until after release :'-) |
(This is more becoming the "let's just implement it damnit" merge request) |
In order to avoid this, I'll cut this PR short on just some "minor" PNI/ACI refactoring, such that https://gitlab.com/whisperfish/whisperfish/-/merge_requests/530#02abb4bfed1d40529ab2bc6cfe9338119e97be98 is also mergable. Continuation in #285. |
Branch as I go along, with things that I disagree with :'-)
One thing I disagree with, but don't necessarily want to fix here:
set_next_prekey_id
sounds like a great source of confusion. In RDBMS-based implementations, thenext_prekey_id
-family is expected to just query the database and return max+1. Setting a value doesn't make any sense, but the naming of the method implies it should need to be a method with a side effect.