-
Notifications
You must be signed in to change notification settings - Fork 9
chore(cat-voices): remove old daos #3745
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
base: feat/database_optimization_3528
Are you sure you want to change the base?
chore(cat-voices): remove old daos #3745
Conversation
📚 Docs PreviewThe docs for this PR can be previewed at the following URL: https://docs.dev.projectcatalyst.io/voices/chore/remove_old_daos |
| /// | ||
| /// Returns the number of deleted rows. | ||
| Future<int> deleteWhere({ | ||
| List<DocumentType>? notInType, |
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.
| List<DocumentType>? notInType, | |
| List<DocumentType>? typeNotIn, |
Suggested one reads:
delete where type not in ...
Current one reads:
delete where not in type ...
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.
Ended up with excludeTypes
| late DriftCatalystDatabase db; | ||
| late DocumentsV2LocalMetadataDao dao; | ||
|
|
||
| setUp(() async { | ||
| final connection = await buildTestConnection(); | ||
| db = DriftCatalystDatabase(connection); | ||
| dao = db.driftDocumentsV2LocalMetadataDao; | ||
| }); | ||
|
|
||
| tearDown(() async { | ||
| await db.close(); | ||
| }); |
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.
Could this be moved inside of the group(DriftDocumentsV2LocalMetadataDao, () {?
| //ignore: one_member_abstracts | ||
| abstract interface class DocumentDataSource { | ||
| Future<DocumentData> get({required DocumentRef ref}); | ||
| Future<DocumentData?> get({DocumentRef? ref}); |
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.
If ref is null, how do you determine which document to return? If it straight throws an error - it's better to require the ref here and throw before this is even called.
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.
You're right, i wanted to make it too generic so i could override it in DocumentDataLocalSource but i'll split it into two methods
| DocumentRef? ref, | ||
| DocumentRef? refTo, |
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.
It's a bit confusing if ref is next to refTo because refTo param translates to ref field from signed doc and ref param translates to id and ver. Clearest would be to have param names be the same as field names but that's not possible. How about renaming ref to selfRef and refTo to ref?
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.
For now, renamed refTo to referencing. I'll deal with ref in separate PR
…talyst-voices into chore/remove_old_daos
* smaller proposals page query * update PR nr
Description
This PR removes old DAOs and tables
Related Issue(s)
Part of #3528
Description of Changes
Please confirm the following checks