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

[cloud_firestore] Add FieldPath #1296

Merged
merged 30 commits into from
Nov 7, 2019
Merged

[cloud_firestore] Add FieldPath #1296

merged 30 commits into from
Nov 7, 2019

Conversation

creativecreatorormaybenot
Copy link
Contributor

@creativecreatorormaybenot creativecreatorormaybenot commented Oct 22, 2019

Description

This brought my attention to FieldPath again. While my solution on StackOverflow works, it is not particularly nice to use.

Related Issues

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • 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 (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • 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?

  • No, this is not a breaking change.

@creativecreatorormaybenot creativecreatorormaybenot changed the title Add FieldPath [cloud_firestore] Add FieldPath Oct 22, 2019
Copy link
Contributor

@collinjackson collinjackson 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 adding tests! It seems like we're relying on an implementation detail (the sentinel value is "__name__") and instead I think we should implement it in the same way as FieldValue and call the native version of FieldPath.documentId.

https://github.com/FirebaseExtended/flutterfire/blob/a24db9c4dac979989b86838069095b95ab2202a9/packages/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java#L917

packages/cloud_firestore/lib/src/field_path.dart Outdated Show resolved Hide resolved
packages/cloud_firestore/example/android/gradle.properties Outdated Show resolved Hide resolved
@creativecreatorormaybenot creativecreatorormaybenot changed the title [cloud_firestore] Add FieldPath [cloud_firestore] Add FieldPath [WIP] Oct 23, 2019
@creativecreatorormaybenot creativecreatorormaybenot changed the title [cloud_firestore] Add FieldPath [WIP] [cloud_firestore] Add FieldPath Oct 23, 2019
@creativecreatorormaybenot
Copy link
Contributor Author

@collinjackson What kind of formatter are you using in flutterfire?

When I use the formatter (which the IDE does automatically 😞), the whole file is changed and the Cirrus CI test also fails.. It is very cumbersome for me to manually adjust the format.

@collinjackson
Copy link
Contributor

collinjackson commented Oct 23, 2019

@collinjackson What kind of formatter are you using in flutterfire?

When I use the formatter (which the IDE does automatically 😞), the whole file is changed and the Cirrus CI test also fails.. It is very cumbersome for me to manually adjust the format.

We're using pub global run flutter_plugin_tools format. That's all I know...

Pro tip: The failure message gives you a patch command that will fix the formatting for you:

patch -p1 <<DONE
diff --git a/packages/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java b/packages/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java
index f231617..524589d 100644
--- a/packages/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java
+++ b/packages/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java
@@ -133,14 +133,15 @@ public class CloudFirestorePlugin implements MethodCallHandler {
         if (field instanceof FieldPath) {
           if (field == FieldPath.documentId()) {
-            throw new IllegalArgumentException("You cannot use {start/end}{At/After}Document " +
-                    "when ordering by the document id. When using any of the mentioned methods, " +
-                    "the library will order by the document id implicitly as " +
-                    "it needs to add other fields to the order clause.");
+            throw new IllegalArgumentException(
+                "You cannot use {start/end}{At/After}Document "
+                    + "when ordering by the document id. When using any of the mentioned methods, "
+                    + "the library will order by the document id implicitly as "
+                    + "it needs to add other fields to the order clause.");
           } else {
             // Unsupported type.
           }
-        } else if (field instanceof  String) {
+        } else if (field instanceof String) {
           String orderByFieldName = (String) field;
           if (orderByFieldName.contains(".")) {
             String[] fieldNameParts = orderByFieldName.split("\\.");
@@ -313,7 +314,7 @@ public class CloudFirestorePlugin implements MethodCallHandler {
       boolean descending = (boolean) order.get(1);
       Query.Direction direction =
-              descending ? Query.Direction.DESCENDING : Query.Direction.ASCENDING;
+          descending ? Query.Direction.DESCENDING : Query.Direction.ASCENDING;
       if (fieldName != null) {
         query = query.orderBy(fieldName, direction);
diff --git a/packages/cloud_firestore/lib/src/field_path.dart b/packages/cloud_firestore/lib/src/field_path.dart
index e144cd0..64cdde8 100644
--- a/packages/cloud_firestore/lib/src/field_path.dart
+++ b/packages/cloud_firestore/lib/src/field_path.dart
@@ -16,5 +16,6 @@ class FieldPath {
   final _FieldPathType type;
   /// The path to the document id, which can be used in queries.
-  static FieldPath get documentId => const FieldPath._(_FieldPathType.documentId);
+  static FieldPath get documentId =>
+      const FieldPath._(_FieldPathType.documentId);
 }
diff --git a/packages/cloud_firestore/lib/src/firestore_message_codec.dart b/packages/cloud_firestore/lib/src/firestore_message_codec.dart
index 1a555fc..9f658cd 100644
--- a/packages/cloud_firestore/lib/src/firestore_message_codec.dart
+++ b/packages/cloud_firestore/lib/src/firestore_message_codec.dart
@@ -34,7 +34,7 @@ class FirestoreMessageCodec extends StandardMessageCodec {
   static const Map<_FieldPathType, int> _kFieldPathCodes =
       <_FieldPathType, int>{
     _FieldPathType.documentId: _kDocumentId,
-      };
+  };
   @override
   void writeValue(WriteBuffer buffer, dynamic value) {
diff --git a/packages/cloud_firestore/test/cloud_firestore_test.dart b/packages/cloud_firestore/test/cloud_firestore_test.dart
index d437202..63a3f06 100755
--- a/packages/cloud_firestore/test/cloud_firestore_test.dart
+++ b/packages/cloud_firestore/test/cloud_firestore_test.dart
@@ -1264,7 +1264,8 @@ bool _deepEquals(dynamic valueA, dynamic valueB) {
   if (valueA is FieldValue) {
     return valueB is FieldValue && _deepEqualsFieldValue(valueA, valueB);
   }
-  if (valueA is FieldPath) return valueB is FieldPath && valueA.type == valueB.type;
+  if (valueA is FieldPath)
+    return valueB is FieldPath && valueA.type == valueB.type;
   return valueA == valueB;
 }
DONE

@creativecreatorormaybenot
Copy link
Contributor Author

@collinjackson Thanks a bunch!

@creativecreatorormaybenot
Copy link
Contributor Author

creativecreatorormaybenot commented Oct 23, 2019

I think that I am done with the new implementation. I added some exceptions that will help users understand what about their queries was incorrect and most importantly added assertions when possible.

@creativecreatorormaybenot
Copy link
Contributor Author

@collinjackson Can you review again?

Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

thanks, this lgtm

@collinjackson collinjackson merged commit 33e0586 into firebase:master Nov 7, 2019
@creativecreatorormaybenot creativecreatorormaybenot deleted the firestore-field-path branch November 7, 2019 23:02
kroikie pushed a commit to collinjackson/flutterfire that referenced this pull request Nov 15, 2019
@firebase firebase locked and limited conversation to collaborators Aug 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants