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.

Sign up for GitHub

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

Jump to bottom

feat(cloud_firestore_odm_generator): Support all serializable types #11365

Merged
merged 43 commits into from
Oct 10, 2023
Merged

feat(cloud_firestore_odm_generator): Support all serializable types #11365

merged 43 commits into from
Oct 10, 2023

Conversation

Rexios80
Copy link
Contributor

@Rexios80 Rexios80 commented Jul 24, 2023 •
edited

Description

Support querying enum fields

Related Issues

#11364

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@Rexios80
Copy link
Contributor Author

We should merge #11361 in first

@rrousselGit
Copy link
Contributor

👋

This needs testing.

rrousselGit
rrousselGit reviewed Jul 31, 2023
View reviewed changes
...cloud_firestore_odm_generator/cloud_firestore_odm_generator_integration_test/lib/simple.dart
@@ -35,6 +35,12 @@ class Model {
final String value;
}

enum TestEnum {
one,
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd need a test which executes the a where/orderBy query and see if they work.
We'll also need an update operation.
And we need to check reads too

@Rexios80
Copy link
Contributor Author

@rrousselGit Let me know what you think of the new way of doing this

@rrousselGit
Copy link
Contributor

This is looking great! Some small test changes and we should be good to go :)

@Rexios80 Rexios80 changed the title feat(cloud_firestore_odm_generator): Add support for querying enum fields feat(cloud_firestore_odm_generator): Support all serializable types Aug 30, 2023
rrousselGit
rrousselGit reviewed Aug 31, 2023
View reviewed changes
packages/cloud_firestore_odm/cloud_firestore_odm_generator/test/document_reference_test.dart
@@ -13,21 +13,18 @@ Future<void> main() async {
);

group('where(arrayContains)', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because those methods are no longer undefined since we generate query methods for everything

@Rexios80
Copy link
Contributor Author

Rexios80 commented Sep 1, 2023

CI is failing but I want to talk about the changes required before fixing it

@rrousselGit
Copy link
Contributor

LGTM once CI is fixed

@Rexios80
Copy link
Contributor Author

@rrousselGit how is this looking?

@rrousselGit
Copy link
Contributor

Sorry I missed your notification. Looking good!

rrousselGit
rrousselGit approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@rrousselGit rrousselGit left a comment

Choose a reason for hiding this comment

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

LGTM

exaby73
exaby73 approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@exaby73 exaby73 left a comment

Choose a reason for hiding this comment

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

LGTM

@rrousselGit rrousselGit merged commit f4c21f8 into firebase:master Oct 10, 2023
13 checks passed
@Rexios80
Copy link
Contributor Author

What's the plan for releasing this?

@gmarizy
Copy link
Contributor

gmarizy commented Oct 18, 2023

First, I would like to thanks @Rexios80 and @rrousselGit for the work. While non-blocking, having a bunch .whereFieldPath(FieldPath.fromString('fieldName'), isEqualTo: MyEnum.myValue.name) partly defeated the type safe promise from ODM IMHO. It was my main pain point regarding ODM, and you solved it. Thank you you two.

But I think you missed something, since models won't generate until you set create_per_field_to_json: true with version 1.0.0-dev.70. If the breaking change is necessary, changelog and documentation should be updated. If it's the case, I started a PR: #11728

LocLt-Mobile pushed a commit to guide-inc-org/guide-flutter_fire that referenced this pull request Oct 29, 2023
@Rexios80 @LocLt-Mobile
feat(cloud_firestore_odm_generator): Support all serializable types (f…
19e44c7 
…irebase#11365)
@firebase firebase locked and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rrousselGit rrousselGit rrousselGit approved these changes

@exaby73 exaby73 exaby73 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Rexios80 @rrousselGit @gmarizy @exaby73