Skip to content
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

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

sealesj
Copy link
Contributor

@sealesj sealesj commented Mar 21, 2024

Use the firestore API for querying tasks and update the firestore task model

Addresses flutter/flutter#144733

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@sealesj sealesj requested a review from keyonghan as a code owner March 21, 2024 19:58
@sealesj sealesj marked this pull request as draft March 21, 2024 20:00
@sealesj
Copy link
Contributor Author

sealesj commented Mar 21, 2024

@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

Copy link
Contributor

@keyonghan keyonghan left a 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/service/firestore.dart Outdated Show resolved Hide resolved
app_dart/lib/src/service/firestore.dart Outdated Show resolved Hide resolved
app_dart/lib/src/service/firestore.dart Outdated Show resolved Hide resolved
app_dart/lib/src/model/firestore/task.dart Outdated Show resolved Hide resolved
@sealesj
Copy link
Contributor Author

sealesj commented Mar 22, 2024

@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 queryRecentTasks function, you need to run dart run build_runner build to regenerate the mocks.

@sealesj sealesj self-assigned this Apr 12, 2024
@sealesj sealesj marked this pull request as ready for review April 23, 2024 20:12
@sealesj
Copy link
Contributor Author

sealesj commented Apr 23, 2024

Additional formatting changes were brought in from dart run build_runner build and can be removed if desired

@sealesj
Copy link
Contributor Author

sealesj commented Apr 23, 2024

@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, dart test passes all tests successfully

@keyonghan
Copy link
Contributor

keyonghan commented Apr 23, 2024

@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, dart test passes all tests successfully

These auto generated files need a reformat.
dart format --line-length=120 <file_name>.dart

@sealesj
Copy link
Contributor Author

sealesj commented Apr 24, 2024

@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, dart test passes all tests successfully

These auto generated files need a reformat. dart format --line-length=120 <file_name>.dart

@keyonghan Thank you for the guidance! Looks like things are resolved now.

/// [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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

@keyonghan keyonghan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants