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

fix(cloud_firestore_odm): add support for searching Enums #8386

Closed
wants to merge 29 commits into from

Conversation

mrorabau
Copy link

@mrorabau mrorabau commented Apr 3, 2022

Description

This PR should add support for searching Enums [.whereMyEnum()]. Previously using an Enum in a model would be stored and retrieved, but then it couldn't be searched on.

Related Issues

Closes #8338

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.

@google-cla
Copy link

google-cla bot commented Apr 3, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@mrorabau mrorabau changed the title fix[odm_generator]: Add support for searching Enums fix: [odm_generator]: Add support for searching Enums Apr 3, 2022
@Salakar Salakar requested a review from rrousselGit April 4, 2022 12:10
@rrousselGit
Copy link
Contributor

rrousselGit commented Apr 4, 2022

Hello!

Could you add tests for this? Thanks 😄

@mrorabau
Copy link
Author

mrorabau commented Apr 6, 2022

@rrousselGit I will try and add a test next week if you would like. I wasn't sure if you wanted to review it first. (I'm out of town at the moment, sorry I can't do it sooner).

@russellwheatley russellwheatley added the blocked: customer-response Waiting for customer response, e.g. more information was requested. label Apr 21, 2022
@google-oss-bot
Copy link

Hey @mrorabau. We need more information to resolve this issue but there hasn't been an update in 7 weekdays. I'm marking the issue as stale and if there are no new updates in the next 7 days I will close it automatically.

If you have more information that will help us get to the bottom of this, just add a comment!

@google-oss-bot google-oss-bot added the Stale Issue with no recent activity label Apr 22, 2022
@mrorabau
Copy link
Author

@google-oss-bot I'm working on the tests (as requested). Having some issues.

@google-oss-bot google-oss-bot added Needs Attention This issue needs maintainer attention. and removed Stale Issue with no recent activity blocked: customer-response Waiting for customer response, e.g. more information was requested. labels Apr 28, 2022
@mrorabau
Copy link
Author

@rrousselGit If you could take a look at this and see what else is needed. I tried to use the same type of tests you had, however it seems that it only tests for expected compile errors.

@Salakar Salakar changed the title fix: [odm_generator]: Add support for searching Enums fix(odm): add support for searching Enums May 16, 2022
@mrorabau
Copy link
Author

@Salakar
Thanks. I'll look into the error (unless you see it quickly). It is strange that I didn't get that on my end before submitting. There was a merge conflict, but I thought I resolved it.

@russellwheatley
Copy link
Member

Hey @mrorabau, I'm not sure what is happening but you've reformatted the entirety of the iOS code base 🤔. Do you mind updating to the latest flutter stable (3.0.0), clang-format & swiftformat and running melos run format, please?

@mrorabau
Copy link
Author

Do you mind updating to the latest flutter stable (3.0.0), clang-format & swiftformat and running melos run format, please?

@rrousselGit
Yeah, I will run it again (I'm on 3.0.0 now). I noticed that it reformatted (almost) everything, but it was the melos run format that did it (I followed the instructions to submit it). Any chance that you have a wider column set somewhere that isn't in the repo? I ask because it seemed that almost all of the format changes were wrapping related.

@mrorabau
Copy link
Author

@rrousselGit
Sorry if I'm bugging you, but a couple of things:

  • The automated apple tests appear to be failing because of an emulator problem
  • The test "test/document_reference_test.dart" works for me, but fails in the automated test

document_reference_test-Works-2022-05-22 200055
document_reference_test-auto-Fails-2022-05-22 200335

  • The Apple/MacOS files still get formatted wrong. Maybe it has something to do with the line-wrap? It seems that the code in master is not wrapped as early as it does for me.
    • flutter_plugin_tools 0.8.6
    • swiftformat 0.49.9
    • clang-format 14.0.4-++20220520113355+5f66e721ec1d-1exp120220520113441.138

@mrorabau
Copy link
Author

mrorabau commented Jul 8, 2022

@rrousselGit
I did a clean checkout and moved only the files I modified and did not run the formatter so the IOS files are no longer modified by whatever in the swift formatter caused it to go crazy.

@rrousselGit
Copy link
Contributor

Hello!
I'll review it next week. I'd like to merge the other pending ODM PRs first

@russellwheatley russellwheatley changed the title fix(odm): add support for searching Enums fix(cloud_firestore_odm): add support for searching Enums Jul 11, 2022
@rrousselGit
Copy link
Contributor

I don't have the permission to push to your PR. Could you fix the conflicts?

@mrorabau
Copy link
Author

mrorabau commented Jul 13, 2022 via email

@@ -413,6 +414,7 @@ extension on DartType {
generic.isDartCoreString ||
generic.isDartCoreBool ||
generic.isDartCoreObject ||
generic.element?.kind == ElementKind.ENUM ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a test for List<MyEnum>. Could you add one? 😄

Copy link
Author

Choose a reason for hiding this comment

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

Remi,

First, I'm sorry for the delay, but I was trying to test the latest build against our project and it took me a while to figure out that we now seem to need a build.yaml in our own project in order to set the create_field_map=true. Otherwise it produces a bunch of errors. I didn't (currently) see this in the docs. I think if this is a requirement going forward it really needs to by highlighted.

I also changed "dart pub get" to "flutter pub get" in melos.yaml/odm:generator_test_build_runner:build (It was giving me a lot of headaches)

I added List to the Nested collection. Is that sufficient for the List test?

Thanks for your support!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your effort!

About create_field_map, it should be optional. Maybe there's something I missed. I'll look into it

About your test, could you maybe move it to example/test_driver & add a test that performs a query?
Such as collection.whereMyEnum(isEqualTo: MyEnum.value) & maybe one for List too (although I think we're lacking arrayContains query for that.
There should be a few tests there that already do that which you could copy-paste

The goal would be to test that the generated work actually works, instead of simply compiling.

@rrousselGit
Copy link
Contributor

It sounds like we'll have to wait for Flutter to be compatible with analyzer 5.2.0 (which json_serializable uses).

@rrousselGit
Copy link
Contributor

It sounds like Flutter is now compatible with analyzer 5.2.0, so I'll resume fixing this :)

@rrousselGit
Copy link
Contributor

rrousselGit commented Nov 28, 2022

Seems like I got confused. test wasn't updated on stable in the end. I guess it'll have to wait more or use master

@mrorabau
Copy link
Author

Seems like I got confused. test wasn't updated on stable in the end. I guess it'll have to wait more or use master

@rrousselGit can you elaborate on this? I think you are saying the fix/compatability in the latest flutter isn't actually working on wasn't combined to master/stable?

@gmarizy
Copy link
Contributor

gmarizy commented Jan 26, 2023

@rrousselGit is Flutter/analyser v5.2+ compatibility issue solved with Flutter 3.7 ?

# Conflicts:
#	.github/workflows/firebase_firestore.yaml
#	.github/workflows/firebase_performance.yaml
#	melos.yaml
#	packages/cloud_firestore_odm/cloud_firestore_odm/example/pubspec.yaml
#	packages/cloud_firestore_odm/cloud_firestore_odm_generator/lib/src/collection_data.dart
#	packages/cloud_firestore_odm/cloud_firestore_odm_generator/pubspec.yaml
#	packages/cloud_firestore_odm/cloud_firestore_odm_generator/pubspec_overrides.yaml
@Lyokone
Copy link
Contributor

Lyokone commented Mar 19, 2024

Closing this PR, since the ODM is now hosted in its own repository: https://github.com/firebaseextended/firestoreodm-flutter

@Lyokone Lyokone closed this Mar 19, 2024
@firebase firebase locked and limited conversation to collaborators Apr 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 [ODM] Cannot search on Enums (Code Generator does not produce where for type enum)
7 participants