Skip to content

Convert FIRGetOptions.initWithSource to factory method. (#655) #711

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

Merged
merged 3 commits into from
Feb 1, 2018
Merged

Convert FIRGetOptions.initWithSource to factory method. (#655) #711

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e53931c
Convert FIRGetOptions.initWithSource to factory method.
rsgowman Jan 25, 2018
d4abca4
source -> optionsWithSource
rsgowman Jan 26, 2018
d049f89
Swift: GetOptions.source(source:) -> source(_:)
rsgowman Feb 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions Firestore/Example/SwiftBuildTest/main.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,11 +228,11 @@ func readDocument(at docRef: DocumentReference) {
func readDocumentWithOptions(at docRef: DocumentReference) {
docRef.getDocument(options:GetOptions.defaultOptions()) { document, error in
}
docRef.getDocument(options:GetOptions(source:GetSource.default)) { document, error in
docRef.getDocument(options:GetOptions.source(source:GetSource.default)) { document, error in
}
docRef.getDocument(options:GetOptions(source:.server)) { document, error in
docRef.getDocument(options:GetOptions.source(source:.server)) { document, error in
}
docRef.getDocument(options:GetOptions(source:GetSource.cache)) { document, error in
docRef.getDocument(options:GetOptions.source(source:GetSource.cache)) { document, error in
}
}

Expand All @@ -249,11 +249,11 @@ func readDocuments(matching query: Query) {
func readDocumentsWithOptions(matching query: Query) {
query.getDocuments(options:GetOptions.defaultOptions()) { querySnapshot, error in
}
query.getDocuments(options:GetOptions.init(source:GetSource.default)) { querySnapshot, error in
query.getDocuments(options:GetOptions.source(source: GetSource.default)) { querySnapshot, error in
}
query.getDocuments(options:GetOptions.init(source:GetSource.server)) { querySnapshot, error in
query.getDocuments(options:GetOptions.source(source: .server)) { querySnapshot, error in
}
query.getDocuments(options:GetOptions.init(source:GetSource.cache)) { querySnapshot, error in
query.getDocuments(options:GetOptions.source(source: GetSource.cache)) { querySnapshot, error in
}
}

Expand Down
58 changes: 23 additions & 35 deletions Firestore/Example/Tests/Integration/API/FIRGetOptionsTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,7 @@ - (void)testGetDocumentWhileOnlineCacheOnly {
// get doc and ensure that it exists, *is* from the cache, and matches
// the initialData.
FIRDocumentSnapshot *result =
[self readDocumentForRef:doc
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceCache]];
[self readDocumentForRef:doc options:[FIRGetOptions optionsWithSource:FIRGetSourceCache]];
XCTAssertTrue(result.exists);
XCTAssertTrue(result.metadata.fromCache);
XCTAssertFalse(result.metadata.hasPendingWrites);
Expand All @@ -167,8 +166,7 @@ - (void)testGetCollectionWhileOnlineCacheOnly {
// get docs and ensure they *are* from the cache, and matches the
// initialDocs.
FIRQuerySnapshot *result =
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceCache]];
[self readDocumentSetForRef:col options:[FIRGetOptions optionsWithSource:FIRGetSourceCache]];
XCTAssertTrue(result.metadata.fromCache);
XCTAssertFalse(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(FIRQuerySnapshotGetData(result), (@[
Expand Down Expand Up @@ -205,8 +203,7 @@ - (void)testGetDocumentWhileOfflineCacheOnly {
// get doc and ensure it exists, *is* from the cache, and matches the
// newData.
FIRDocumentSnapshot *result =
[self readDocumentForRef:doc
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceCache]];
[self readDocumentForRef:doc options:[FIRGetOptions optionsWithSource:FIRGetSourceCache]];
XCTAssertTrue(result.exists);
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
Expand Down Expand Up @@ -237,8 +234,7 @@ - (void)testGetCollectionWhileOfflineCacheOnly {
// get docs and ensure they *are* from the cache, and matches the updated
// data.
FIRQuerySnapshot *result =
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceCache]];
[self readDocumentSetForRef:col options:[FIRGetOptions optionsWithSource:FIRGetSourceCache]];
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(FIRQuerySnapshotGetData(result), (@[
Expand All @@ -264,8 +260,7 @@ - (void)testGetDocumentWhileOnlineServerOnly {
// get doc and ensure that it exists, is *not* from the cache, and matches
// the initialData.
FIRDocumentSnapshot *result =
[self readDocumentForRef:doc
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceServer]];
[self readDocumentForRef:doc options:[FIRGetOptions optionsWithSource:FIRGetSourceServer]];
XCTAssertTrue(result.exists);
XCTAssertFalse(result.metadata.fromCache);
XCTAssertFalse(result.metadata.hasPendingWrites);
Expand All @@ -286,8 +281,7 @@ - (void)testGetCollectionWhileOnlineServerOnly {
// get docs and ensure they are *not* from the cache, and matches the
// initialData.
FIRQuerySnapshot *result =
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceServer]];
[self readDocumentSetForRef:col options:[FIRGetOptions optionsWithSource:FIRGetSourceServer]];
XCTAssertFalse(result.metadata.fromCache);
XCTAssertFalse(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(FIRQuerySnapshotGetData(result), (@[
Expand All @@ -314,7 +308,7 @@ - (void)testGetDocumentWhileOfflineServerOnly {

// attempt to get doc and ensure it cannot be retreived
XCTestExpectation *failedGetDocCompletion = [self expectationWithDescription:@"failedGetDoc"];
[doc getDocumentWithOptions:[[FIRGetOptions alloc] initWithSource:FIRGetSourceServer]
[doc getDocumentWithOptions:[FIRGetOptions optionsWithSource:FIRGetSourceServer]
completion:^(FIRDocumentSnapshot *snapshot, NSError *error) {
XCTAssertNotNil(error);
XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
Expand All @@ -340,7 +334,7 @@ - (void)testGetCollectionWhileOfflineServerOnly {

// attempt to get docs and ensure they cannot be retreived
XCTestExpectation *failedGetDocsCompletion = [self expectationWithDescription:@"failedGetDocs"];
[col getDocumentsWithOptions:[[FIRGetOptions alloc] initWithSource:FIRGetSourceServer]
[col getDocumentsWithOptions:[FIRGetOptions optionsWithSource:FIRGetSourceServer]
completion:^(FIRQuerySnapshot *snapshot, NSError *error) {
XCTAssertNotNil(error);
XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
Expand Down Expand Up @@ -380,24 +374,23 @@ - (void)testGetDocumentWhileOfflineWithDifferentGetOptions {
// get doc (from cache) and ensure it exists, *is* from the cache, and
// matches the newData.
FIRDocumentSnapshot *result =
[self readDocumentForRef:doc
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceCache]];
[self readDocumentForRef:doc options:[FIRGetOptions optionsWithSource:FIRGetSourceCache]];
XCTAssertTrue(result.exists);
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(result.data, newData);

// attempt to get doc (with default get options)
result = [self readDocumentForRef:doc
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceDefault]];
result =
[self readDocumentForRef:doc options:[FIRGetOptions optionsWithSource:FIRGetSourceDefault]];
XCTAssertTrue(result.exists);
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(result.data, newData);

// attempt to get doc (from the server) and ensure it cannot be retreived
XCTestExpectation *failedGetDocCompletion = [self expectationWithDescription:@"failedGetDoc"];
[doc getDocumentWithOptions:[[FIRGetOptions alloc] initWithSource:FIRGetSourceServer]
[doc getDocumentWithOptions:[FIRGetOptions optionsWithSource:FIRGetSourceServer]
completion:^(FIRDocumentSnapshot *snapshot, NSError *error) {
XCTAssertNotNil(error);
XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
Expand Down Expand Up @@ -440,8 +433,7 @@ - (void)testGetCollectionWhileOfflineWithDifferentGetOptions {
// get docs (from cache) and ensure they *are* from the cache, and
// matches the updated data.
FIRQuerySnapshot *result =
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceCache]];
[self readDocumentSetForRef:col options:[FIRGetOptions optionsWithSource:FIRGetSourceCache]];
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(FIRQuerySnapshotGetData(result), (@[
Expand All @@ -458,7 +450,7 @@ - (void)testGetCollectionWhileOfflineWithDifferentGetOptions {

// attempt to get docs (with default get options)
result = [self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceDefault]];
options:[FIRGetOptions optionsWithSource:FIRGetSourceDefault]];
XCTAssertTrue(result.metadata.fromCache);
XCTAssertEqualObjects(FIRQuerySnapshotGetData(result), (@[
@{@"key1" : @"value1"}, @{@"key2" : @"value2", @"key2b" : @"value2b"},
Expand All @@ -474,7 +466,7 @@ - (void)testGetCollectionWhileOfflineWithDifferentGetOptions {

// attempt to get docs (from the server) and ensure they cannot be retreived
XCTestExpectation *failedGetDocsCompletion = [self expectationWithDescription:@"failedGetDocs"];
[col getDocumentsWithOptions:[[FIRGetOptions alloc] initWithSource:FIRGetSourceServer]
[col getDocumentsWithOptions:[FIRGetOptions optionsWithSource:FIRGetSourceServer]
completion:^(FIRQuerySnapshot *snapshot, NSError *error) {
XCTAssertNotNil(error);
XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
Expand Down Expand Up @@ -547,7 +539,7 @@ - (void)testGetNonExistingDocWhileOnlineCacheOnly {
// certain documents *don't* exist.
XCTestExpectation *getNonExistingDocCompletion =
[self expectationWithDescription:@"getNonExistingDoc"];
[doc getDocumentWithOptions:[[FIRGetOptions alloc] initWithSource:FIRGetSourceCache]
[doc getDocumentWithOptions:[FIRGetOptions optionsWithSource:FIRGetSourceCache]
completion:^(FIRDocumentSnapshot *snapshot, NSError *error) {
XCTAssertNotNil(error);
XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
Expand All @@ -562,8 +554,7 @@ - (void)testGetNonExistingCollectionWhileOnlineCacheOnly {

// get collection and ensure it's empty and that it *is* from the cache.
FIRQuerySnapshot *snapshot =
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceCache]];
[self readDocumentSetForRef:col options:[FIRGetOptions optionsWithSource:FIRGetSourceCache]];
XCTAssertEqual(snapshot.count, 0);
XCTAssertEqual(snapshot.documentChanges.count, 0);
XCTAssertTrue(snapshot.metadata.fromCache);
Expand All @@ -581,7 +572,7 @@ - (void)testGetNonExistingDocWhileOfflineCacheOnly {
// certain documents *don't* exist.
XCTestExpectation *getNonExistingDocCompletion =
[self expectationWithDescription:@"getNonExistingDoc"];
[doc getDocumentWithOptions:[[FIRGetOptions alloc] initWithSource:FIRGetSourceCache]
[doc getDocumentWithOptions:[FIRGetOptions optionsWithSource:FIRGetSourceCache]
completion:^(FIRDocumentSnapshot *snapshot, NSError *error) {
XCTAssertNotNil(error);
XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
Expand All @@ -599,8 +590,7 @@ - (void)testGetNonExistingCollectionWhileOfflineCacheOnly {

// get collection and ensure it's empty and that it *is* from the cache.
FIRQuerySnapshot *snapshot =
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceCache]];
[self readDocumentSetForRef:col options:[FIRGetOptions optionsWithSource:FIRGetSourceCache]];
XCTAssertEqual(snapshot.count, 0);
XCTAssertEqual(snapshot.documentChanges.count, 0);
XCTAssertTrue(snapshot.metadata.fromCache);
Expand All @@ -612,8 +602,7 @@ - (void)testGetNonExistingDocWhileOnlineServerOnly {

// get doc and ensure that it does not exist and is *not* from the cache.
FIRDocumentSnapshot *snapshot =
[self readDocumentForRef:doc
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceServer]];
[self readDocumentForRef:doc options:[FIRGetOptions optionsWithSource:FIRGetSourceServer]];
XCTAssertFalse(snapshot.exists);
XCTAssertFalse(snapshot.metadata.fromCache);
XCTAssertFalse(snapshot.metadata.hasPendingWrites);
Expand All @@ -624,8 +613,7 @@ - (void)testGetNonExistingCollectionWhileOnlineServerOnly {

// get collection and ensure that it's empty and that it's *not* from the cache.
FIRQuerySnapshot *snapshot =
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRGetSourceServer]];
[self readDocumentSetForRef:col options:[FIRGetOptions optionsWithSource:FIRGetSourceServer]];
XCTAssertEqual(snapshot.count, 0);
XCTAssertEqual(snapshot.documentChanges.count, 0);
XCTAssertFalse(snapshot.metadata.fromCache);
Expand All @@ -643,7 +631,7 @@ - (void)testGetNonExistingDocWhileOfflineServerOnly {
// certain documents *don't* exist.
XCTestExpectation *getNonExistingDocCompletion =
[self expectationWithDescription:@"getNonExistingDoc"];
[doc getDocumentWithOptions:[[FIRGetOptions alloc] initWithSource:FIRGetSourceServer]
[doc getDocumentWithOptions:[FIRGetOptions optionsWithSource:FIRGetSourceServer]
completion:^(FIRDocumentSnapshot *snapshot, NSError *error) {
XCTAssertNotNil(error);
XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
Expand All @@ -661,7 +649,7 @@ - (void)testGetNonExistingCollectionWhileOfflineServerOnly {

// attempt to get collection and ensure that it cannot be retreived
XCTestExpectation *failedGetDocsCompletion = [self expectationWithDescription:@"failedGetDocs"];
[col getDocumentsWithOptions:[[FIRGetOptions alloc] initWithSource:FIRGetSourceServer]
[col getDocumentsWithOptions:[FIRGetOptions optionsWithSource:FIRGetSourceServer]
completion:^(FIRQuerySnapshot *snapshot, NSError *error) {
XCTAssertNotNil(error);
XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
Expand Down
2 changes: 2 additions & 0 deletions Firestore/Source/API/FIRGetOptions+Internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ NS_ASSUME_NONNULL_BEGIN
/** Where getDocument[s] calls should get their data from. */
@property(nonatomic, readonly, getter=source) FIRGetSource source;

- (instancetype)initWithSource:(FIRGetSource)source;

@end

NS_ASSUME_NONNULL_END
6 changes: 5 additions & 1 deletion Firestore/Source/API/FIRGetOptions.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

@implementation FIRGetOptions

+ (FIRGetOptions *)defaultOptions {
+ (instancetype)defaultOptions {
return [[FIRGetOptions alloc] initWithSource:FIRGetSourceDefault];
}

Expand All @@ -31,6 +31,10 @@ - (instancetype)initWithSource:(FIRGetSource)source {
return self;
}

+ (instancetype)optionsWithSource:(FIRGetSource)source {
return [[FIRGetOptions alloc] initWithSource:source];
}

@end

NS_ASSUME_NONNULL_END
9 changes: 6 additions & 3 deletions Firestore/Source/Public/FIRGetOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,16 @@ NS_ASSUME_NONNULL_BEGIN
NS_SWIFT_NAME(GetOptions)
@interface FIRGetOptions : NSObject

/** */
- (instancetype)init __attribute((unavailable("FIRGetOptions cannot be created directly.")));

/**
* Returns the default options.
*
* Equiavlent to `[[FIRGetOptions alloc] initWithSource:FIRGetSourceDefault]` in
* Equiavlent to `[FIRGetOptions source:FIRGetSourceDefault]` in
* objective-c.
*/
+ (FIRGetOptions *)defaultOptions NS_SWIFT_NAME(defaultOptions());
+ (instancetype)defaultOptions NS_SWIFT_NAME(defaultOptions());

/**
* Describes whether we should get from server or cache.
Expand Down Expand Up @@ -62,7 +65,7 @@ typedef NS_ENUM(NSUInteger, FIRGetSource) {
/**
* Initializes the get options with the specified source.
*/
- (instancetype)initWithSource:(FIRGetSource)source NS_SWIFT_NAME(init(source:));
+ (instancetype)optionsWithSource:(FIRGetSource)source NS_SWIFT_NAME(source(source:));
Copy link
Member

Choose a reason for hiding this comment

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

I think leaving this as a constructor would be good instead of fighting against the translator: GetOptions(source: .cache).

Copy link
Contributor

Choose a reason for hiding this comment

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

So the issue is that we're going to add a timeout too.

In the other languages you'll be able to do things like this:

GetOptions.source(GetOptions.Source.CACHE).timeout(2, TimeUnit.MINUTES);

GetOptions.timeout(2, TimeUnit.MINUTES).source(GetOptions.Source.CACHE);

Making this an initializer only thing means that the same flexibility would require us to define all the permutations of options which doesn't seem like what we want to do at all.

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh okay, that makes sense. This wouldn't solve it entirely though, would it? You'd be left with an instance instead and unable to call the next class method. Unless each option was a constructor and a function:

class func source(_ source: GetSource) -> GetOptions
class func timeout(_ value: Int, _ unit: TimeUnit) -> GetOptions

func source(_ source: GetSource) -> GetOptions
func timeout(_ value: Int, _ unit: TimeUnit) -> GetOptions

Do you mean to have this as an instance method, or am I missing something?

It's worth noting that this could potentially go away once we have the mechanism to ship native Swift files:

public func init(source: GetOptions.Source = .default, 
                 timeout: (Int, TimeUnit) = (2, .minutes)) 
{                   // (or have Timeout as its own type)
  // Implementation
}

// Usage:
GetOptions()
GetOptions(source: .cache)
GetOptions(timeout: (5, .seconds))
GetOptions(source: .cache, timeout: (8, .seconds))

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think the idea would be to add instance methods as appropriate to allow chaining.

I like the idea of (eventually) using named parameters. (There's a concern if we ever have a very large number of options, but I don't think that's the case here.) Is there an approx timeline for being able to do this?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the API with one minor change to match SnapshotOptions: remove the argument label to make it source(_:), which turns the usage into: GetSource.source(.cache). I would be interested to see how it's going to expand with multiple parameters though - class and instance functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done; ptal.

(And yeah; expected path forward is to expand with class and instance functions.)


@end

Expand Down