-
Notifications
You must be signed in to change notification settings - Fork 487
DatabaseUI Tests: Road to green CI #108
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
Changes from all commits
dbfd6ed
218a38a
1c44ad7
0617d42
8a73485
1c39d2e
88709f8
2efcf35
bfd3b63
a455819
6348024
f29ce42
696d31b
0d01485
9d962c1
c7da9ae
e57e70b
bca666a
82383bf
b7488f9
577a3d7
89efa98
1be8364
e624ecf
2797943
a2473da
fa2918a
a100271
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,15 +18,25 @@ | |
|
|
||
| // clang-format on | ||
|
|
||
| #import <Foundation/Foundation.h> | ||
| @import Firebase; | ||
|
|
||
| #import "FirebaseArrayDelegate.h" | ||
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| @class FIRDatabaseQuery; | ||
| @class FIRDatabaseReference; | ||
| @class FIRDataSnapshot; | ||
| @protocol FIRDataObservable | ||
| @required | ||
|
|
||
| - (FIRDatabaseHandle)observeEventType:(FIRDataEventType)eventType | ||
| andPreviousSiblingKeyWithBlock:(void (^)(FIRDataSnapshot *snapshot, NSString *__nullable prevKey))block | ||
| withCancelBlock:(nullable void (^)(NSError* error))cancelBlock; | ||
|
|
||
| - (void)removeObserverWithHandle:(FIRDatabaseHandle)handle; | ||
|
|
||
| @end | ||
|
|
||
| @interface FIRDatabaseQuery (FIRDataObservable) <FIRDataObservable> | ||
| @end | ||
|
|
||
| /** | ||
| * FirebaseArray provides an array structure that is synchronized with a Firebase reference or | ||
|
|
@@ -39,43 +49,36 @@ NS_ASSUME_NONNULL_BEGIN | |
| * The delegate object that array changes are surfaced to, which conforms to the | ||
| * [FirebaseArrayDelegate Protocol](FirebaseArrayDelegate). | ||
| */ | ||
| @property(weak, nonatomic) id<FirebaseArrayDelegate> delegate; | ||
| @property(weak, nonatomic, nullable) id<FirebaseArrayDelegate> delegate; | ||
|
|
||
| /** | ||
| * The query on a Firebase reference that provides data to populate the instance of FirebaseArray. | ||
| */ | ||
| @property(strong, nonatomic) FIRDatabaseQuery *query; | ||
| @property(strong, nonatomic) id<FIRDataObservable> query; | ||
|
|
||
| /** | ||
| * The delegate object that array changes are surfaced to. | ||
| * The number of objects in the FirebaseArray. | ||
| */ | ||
| @property(strong, nonatomic) NSMutableArray<FIRDataSnapshot *> * snapshots; | ||
|
|
||
| #pragma mark - | ||
| #pragma mark Initializer methods | ||
| @property(nonatomic, readonly) NSUInteger count; | ||
|
|
||
| /** | ||
| * Intitalizes FirebaseArray with a standard Firebase reference. | ||
| * @param ref The Firebase reference which provides data to FirebaseArray | ||
| * @return The instance of FirebaseArray | ||
| * The items currently in the FirebaseArray. | ||
| */ | ||
| - (instancetype)initWithRef:(FIRDatabaseReference *)ref; | ||
| @property(nonatomic, readonly, copy) NSArray *items; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does copy make sense here with readonly? I notice you have the same set of params earlier as well, but in a different order: reorder one set. May be worth checking the Google objective C style guide to see if there is an ordering recommendation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it makes sense to keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe copy copies on read, only write. Regarding the style, but in the absence of a known community opinion we should prefer existing style guides. If we diverge from that, we should note that we are, and document our own style variations. Regardless of what we choose, we should be consistent - so either change this one to match the order in https://github.com/firebase/FirebaseUI-iOS/pull/108/files#diff-13a906499860a93cf34277f908dfb0b7R37 or vice versa. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation manually copies on read, since the backing array is mutable. I guess I could just delete copy since it doesn't do anything here. |
||
|
|
||
| #pragma mark - Initializer methods | ||
|
|
||
| /** | ||
| * Intitalizes FirebaseArray with a Firebase query (FIRDatabaseQuery). | ||
| * @param query A query on a Firebase reference which provides filtered data to FirebaseArray | ||
| * @return The instance of FirebaseArray | ||
| * Initalizes FirebaseArray with a Firebase query (FIRDatabaseQuery) or database reference | ||
| * (FIRDatabaseReference). | ||
| * @param query A query or Firebase database reference | ||
| * @return A FirebaseArray instance | ||
| */ | ||
| - (instancetype)initWithQuery:(FIRDatabaseQuery *)query; | ||
| - (instancetype)initWithQuery:(id<FIRDataObservable>)query NS_DESIGNATED_INITIALIZER; | ||
|
|
||
| #pragma mark - | ||
| #pragma mark Public API methods | ||
| - (instancetype)init NS_UNAVAILABLE; | ||
|
|
||
| /** | ||
| * Returns the count of objects in the FirebaseArray. | ||
| * @return The count of objects in the FirebaseArray | ||
| */ | ||
| - (NSUInteger)count; | ||
| #pragma mark - Public API methods | ||
|
|
||
| /** | ||
| * Returns an object at a specific index in the FirebaseArray. | ||
|
|
@@ -100,21 +103,16 @@ NS_ASSUME_NONNULL_BEGIN | |
|
|
||
| /** | ||
| * Support for subscripting. This method is unused and trying to write directly to the | ||
| * array using subscripting will cause an assertion. | ||
| * array using subscripting will cause an assertion failure. | ||
| */ | ||
| - (void)setObject:(id)obj atIndexedSubscript:(NSUInteger)idx; | ||
|
|
||
| #pragma mark - | ||
| #pragma mark Private API methods | ||
| - (void)setObject:(id)obj atIndexedSubscript:(NSUInteger)idx NS_UNAVAILABLE; | ||
|
|
||
| /** | ||
| * Returns an index for a given object's key (that matches the object's key in the corresponding | ||
| * Firebase reference). | ||
| * @param key The key of the desired object | ||
| * @return The index of the object for which the key matches or -1 if the key is null | ||
| * @exception FirebaseArrayKeyNotFoundException Thrown when the desired key is not in the | ||
| * FirebaseArray, likely indicating that the FirebaseArray is no longer being properly synchronized | ||
| * with the Firebase database. | ||
| * @return The index of the object for which the key matches or NSNotFound if the key is not found | ||
| * @exception NSInvalidArgumentException Thrown when the `key` parameter is `nil`. | ||
| */ | ||
| - (NSUInteger)indexForKey:(NSString *)key; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,8 @@ | |
|
|
||
| // clang-format on | ||
|
|
||
| @class FirebaseArray; | ||
|
|
||
| /** | ||
| * A protocol to allow instances of FirebaseArray to raise events through a | ||
| * delegate. Raises all | ||
|
|
@@ -36,7 +38,7 @@ | |
| * @param object The object added to the FirebaseArray | ||
| * @param index The index the child was added at | ||
| */ | ||
| - (void)childAdded:(id)object atIndex:(NSUInteger)index; | ||
| - (void)array:(FirebaseArray *)array didAddObject:(id)object atIndex:(NSUInteger)index; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes are pretty impactful I think, people do use FirebaseArray directly. We may want a migration cheat sheet - replace X with Y kind of thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW we consider FirebaseArray to be a "private API" even though some are accessing it. So we can flag it in the changelog but we don't have to worry too much. On Android it's not possible to use FirebaseArray directly due to the access modifiers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In our source comments we suggest users use FirebaseArray to build on top of, so I think it's worth treating as public. Also it's extremely confusing to tell users that something exposed by the module as public is actually not supposed to be public. |
||
|
|
||
| /** | ||
| * Delegate method which is called whenever an object is chinged in a | ||
|
|
@@ -47,7 +49,7 @@ | |
| * @param object The object that changed in the FirebaseArray | ||
| * @param index The index the child was changed at | ||
| */ | ||
| - (void)childChanged:(id)object atIndex:(NSUInteger)index; | ||
| - (void)array:(FirebaseArray *)array didChangeObject:(id)object atIndex:(NSUInteger)index; | ||
|
|
||
| /** | ||
| * Delegate method which is called whenever an object is removed from a | ||
|
|
@@ -58,7 +60,7 @@ | |
| * @param object The object removed from the FirebaseArray | ||
| * @param index The index the child was removed at | ||
| */ | ||
| - (void)childRemoved:(id)object atIndex:(NSUInteger)index; | ||
| - (void)array:(FirebaseArray *)array didRemoveObject:(id)object atIndex:(NSUInteger)index; | ||
|
|
||
| /** | ||
| * Delegate method which is called whenever an object is moved within a | ||
|
|
@@ -70,12 +72,12 @@ | |
| * @param fromIndex The index the child is being moved from | ||
| * @param toIndex The index the child is being moved to | ||
| */ | ||
| - (void)childMoved:(id)object fromIndex:(NSUInteger)fromIndex toIndex:(NSUInteger)toIndex; | ||
| - (void)array:(FirebaseArray *)array didMoveObject:(id)object fromIndex:(NSUInteger)fromIndex toIndex:(NSUInteger)toIndex; | ||
|
|
||
| /** | ||
| * Delegate method which is called whenever the backing query is canceled. | ||
| * @param error the error that was raised | ||
| */ | ||
| - (void)canceledWithError:(NSError *)error; | ||
| - (void)array:(FirebaseArray *)array queryCancelledWithError:(NSError *)error; | ||
|
|
||
| @end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is a behavior change I think, though the effective difference between those scopes is slight, an upgrade may cause a reconsent. At the least we should flag this in changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefix has been deleted because it was defined before but never used.