-
Notifications
You must be signed in to change notification settings - Fork 96
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
Firestore API migration for vacuum_stale_tasks #3587
base: main
Are you sure you want to change the base?
Conversation
@keyonghan This is an initial draft, but is this what you have in mind for the Task updates for firestore? I am wondering because with these updates I would also need to change use of Task to firestore in a few other places as well |
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.
Thanks for the contribution. Let's also add unit tests to cover the new logics.
app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart
Outdated
Show resolved
Hide resolved
app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart
Outdated
Show resolved
Hide resolved
@keyonghan For testing are you thinking that the firestore service needs test for the queryRecentTasks? I can add that test with a firestore setup of initial commits if so. I am thinking unit tests to cover logic changes in vaccum_stale_tasks.dart. BTW: for the newly added |
…re-vacuum-migration
…re-vacuum-migration
Additional formatting changes were brought in from |
@keyonghan I've added the tests but am still seeing the Linux Cocoon check failing and am not sure why. It seems to only be showing formatting changes? Locally, |
These auto generated files need a reformat. |
@keyonghan Thank you for the guidance! Looks like things are resolved now. |
app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart
Outdated
Show resolved
Hide resolved
/// [Commit] object (just the relational mapping). This class exists for those | ||
/// times when the caller has loaded the associated commit from firestore | ||
/// and would like to pass both the task its commit around. | ||
class FullTask { |
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.
Just a thought and no pressue, one thing to possibly consider is simply using Records rather than defining a full class.
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'm new to using records but really like the idea. I updated to use a record instead of FullTask, hopefully it is a bit simpler
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
Use the firestore API for querying tasks and update the firestore task model
Addresses flutter/flutter#144733
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.