Support for joins #164

morganchen12 · 2016-10-10T23:22:20Z

Mirrors firebase/FirebaseUI-Android#276.

~~Still missing a few things:~~

~~Documentation~~
~~Actual error handling~~
~~Data source tests~~
~~Tests for edge cases~~

Good to merge, pending some code review.

ianbarber · 2016-10-12T20:27:48Z

FirebaseDatabaseUI/FirebaseIndexArray.h

+/**
+ * A protocol to allow instances of FirebaseArray to raise events through a
+ * delegate. Raises all
+ * Firebase events except FIRDataEventTypeValue.


nit: weird line split

ianbarber · 2016-10-12T22:01:34Z

FirebaseDatabaseUI/FirebaseIndexArray.h

+ * Delegate method which is called whenever an object is added to a
+ * FirebaseArray. On a
+ * FirebaseArray synchronized to a Firebase reference, this corresponds to an
+ * [FIRDataEventTypeChildAdded](https://www.firebase.com/docs/ios/guide/retrieving-data.html#section-event-types)


Please replace these doc links with ones to firebase.google.com across the file

ianbarber · 2016-10-12T22:02:26Z

FirebaseDatabaseUI/FirebaseIndexArray.h

+
+/**
+ * Delegate method which is called whenever the backing query is canceled. This error is fatal
+ * and the index array will become unusable afterward, so please handle it appropriately.


What is the appropriate handling? Guessing effectively to stop using this object

ianbarber · 2016-10-12T22:04:33Z

FirebaseDatabaseUI/FirebaseIndexArray.m

+
+@interface FirebaseIndexArray () <FirebaseArrayDelegate>
+
+@property (nonatomic, readonly) id<FIRDataObservable> index;


Not a big fan of the readonly here - if we're declaring private property decls lets make them readwrite and use self.property when setting.

Note: discussed this separately, so either way works as long as consistent.

ianbarber · 2016-10-12T22:04:48Z

FirebaseDatabaseUI/FirebaseIndexArray.m

+  NSParameterAssert(data != nil);
+  self = [super init];
+  if (self != nil) {
+    _index = index;


as with comment above, would prefer self.index

ianbarber · 2016-10-12T22:05:33Z

FirebaseDatabaseUI/FirebaseIndexArray.m

+}
+
+- (NSUInteger)count {
+  return self.observers.count;


while here _observers.count would be fine

ianbarber · 2016-10-12T22:07:32Z

FirebaseDatabaseUI/FirebaseIndexArray.m

+#pragma mark - FirebaseArrayDelegate
+
+// These delegate methods are entirely responsible
+// for keeping a local array of queries up to date.


This comment is a bit abandoned here. I might be inclined to move it into a top level class comment that explains what we are trying to do and how we go about it.

ianbarber · 2016-10-12T22:17:02Z

FirebaseDatabaseUI/FirebaseIndexArray.h

+
+@end
+
+@interface FirebaseIndexArray : NSObject


This needs a decent class comment explaining what this is for and how it should be used

ianbarber · 2016-10-12T22:25:47Z

FirebaseDatabaseUI/FirebaseIndexArray.m

+
+@end
+
+@implementation FirebaseIndexArray


Add class comment about any implementation level details (see comment below)

ianbarber · 2016-10-12T22:26:40Z

FirebaseDatabaseUI/FirebaseIndexArray.m

+- (instancetype)initWithIndex:(id<FIRDataObservable>)index
+                         data:(id<FIRDataObservable>)data
+                     delegate:(nullable id<FirebaseIndexArrayDelegate>)delegate; {
+  NSParameterAssert(index != nil);


As discussed, see whether replacing one firebasearray with a query makes the code easier.

I tried this and ended up duplicating much of the index logic since queries only give you the previousChildKey. Might be worth extracting that from FirebaseArray into a more inert data object, but for now I think keeping FirebaseArray here is cleaner, if not less verbose.

morganchen12 · 2016-10-21T22:35:28Z

@ianbarber ready for more review

ianbarber

LGTM, couple of small comment nits.

ianbarber · 2016-10-22T01:21:05Z

FirebaseDatabaseUI/FirebaseArrayDelegate.h

- * [FIRDataEventTypeChildAdded](https://www.firebase.com/docs/ios/guide/retrieving-data.html#section-event-types)
- * event being raised.
+ * FirebaseArray. On a FirebaseArray synchronized to a Firebase reference, 
+ * this corresponds to a @c FIRDataEventTypeChildAdded event being raised.
 * @param object The object added to the FirebaseArray
 * @param index The index the child was added at
 */
 - (void)array:(FirebaseArray *)array didAddObject:(id)object atIndex:(NSUInteger)index;

 /**
 * Delegate method which is called whenever an object is chinged in a


Not your fault but chinged -> changed

ianbarber · 2016-10-22T01:25:05Z

FirebaseDatabaseUI/FirebaseIndexArray.h

+
+/**
+ * Initializes a FirebaseIndexArray with an index query and a data query.
+ * The array expects the keys of the children of the index query to be children


(here and elsewhere) change "to be" to "to match" - this is complex stuff, and clarifying that we have the same values in two places is probably pertinent.

ianbarber · 2016-10-22T01:25:42Z

FirebaseDatabaseUI/FirebaseIndexArray.m

+- (instancetype)initWithIndex:(id<FIRDataObservable>)index
+                         data:(id<FIRDataObservable>)data
+                     delegate:(nullable id<FirebaseIndexArrayDelegate>)delegate; {
+  NSParameterAssert(index != nil);


morganchen12 changed the base branch from master to v0.6.2 October 11, 2016 17:53

morganchen12 closed this Oct 11, 2016

morganchen12 reopened this Oct 11, 2016

morganchen12 closed this Oct 11, 2016

morganchen12 reopened this Oct 11, 2016

morganchen12 force-pushed the joins branch 3 times, most recently from 7e2b1b6 to 8f19ca8 Compare October 12, 2016 17:44

ianbarber reviewed Oct 12, 2016

View reviewed changes

TwitRco approved these changes Oct 12, 2016

View reviewed changes

morganchen12 force-pushed the joins branch 4 times, most recently from 808e654 to aa3a395 Compare October 14, 2016 17:00

morganchen12 added this to the 1.0.0 milestone Oct 17, 2016

morganchen12 added the database label Oct 17, 2016

morganchen12 force-pushed the joins branch 5 times, most recently from ac570ab to ed8267e Compare October 21, 2016 21:43

morganchen12 changed the base branch from v0.6.2 to master October 21, 2016 21:46

ianbarber approved these changes Oct 22, 2016

View reviewed changes

morganchen12 force-pushed the joins branch 2 times, most recently from bc1bb60 to f11270f Compare October 25, 2016 22:35

morganchen12 added 4 commits October 25, 2016 16:37

support for joins

ad34bc7

clean up whitespace

6cf9769

clean up nullability

68d52cd

add data source classes

43a9247

morganchen12 added 13 commits October 25, 2016 16:37

clean up FirebaseIndexArray

625a774

implement delegate methods, add doc comments

c3d0abd

fix uninitialized variable in block capture

9b26087

implement error handling methods for indexed data sources

84880a1

add FirebaseIndexTableViewDataSourceTests

6ad8a89

add FirebaseIndexCollectionViewDataSourceTest

6628abd

fix bad links in doc comments

09045ac

update doc comments

5b3d50d

simplify object ownership graph and update doc comments

9b8c842

add tests for index array delegate, edge cases

90bf0c4

disable documentation warnings, fix some docs issues

f79764a

fix up doc comments

1906314

fix tests not loading dylibs correctly

0543fa7

morganchen12 force-pushed the joins branch from f11270f to 0543fa7 Compare October 25, 2016 23:38

remove cell creation from data sources

8619311

morganchen12 changed the base branch from master to v1.0.0 November 1, 2016 23:08

morganchen12 merged commit 596afb0 into firebase:v1.0.0 Nov 1, 2016

SUPERCILEX mentioned this pull request Nov 1, 2016

Support for joins firebase/FirebaseUI-Android#276

Merged

1 task


		@interface FirebaseIndexArray () <FirebaseArrayDelegate>

		@property (nonatomic, readonly) id<FIRDataObservable> index;


		@end

		@implementation FirebaseIndexArray

Support for joins #164

Support for joins #164

Uh oh!

Conversation

morganchen12 commented Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganchen12 commented Oct 21, 2016

Uh oh!

ianbarber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

morganchen12 commented Oct 10, 2016 •

edited

Loading