Make FSTTimestamp into a public class: #698

var-const · 2018-01-24T20:13:33Z

rename to FIRTimestamp and move into Public/;
split out internal part of FIRTimestamp;
hide the fact that implementation uses nanoseconds - only show
microseconds in interface and truncate sub-microsecond precision if
FIRTimestamp is created from NSDate.

- rename to FIRTimestamp and move into Public/; - split out internal part of FIRTimestamp; - hide the fact that implementation uses nanoseconds - only show microseconds in interface and truncate sub-microsecond precision if FIRTimestamp is created from NSDate.

var-const · 2018-01-24T20:27:39Z

Gil, this is using microseconds, so I presume it's obsolete, but I'll still appreciate any feedback.

wilhuff

The changes to get from micros to nanos on top of this change would be pretty small, so let's press on.

Thanks for working through this change so far!

wilhuff · 2018-01-25T08:52:19Z

Firestore/Example/Tests/Integration/API/FIRTimestampAsArgumentTests.m

@@ -0,0 +1,149 @@
+/*
+ * Copyright 2017 Google


New file, new year :-)

wilhuff · 2018-01-25T08:53:37Z

Firestore/Example/Tests/Integration/API/FIRTimestampAsArgumentTests.m

+
+#import <XCTest/XCTest.h>
+
+#import "FirebaseFirestore/FIRTimestamp.h"


FYI framework imports should be in angle brackets.

However, this should be unnecessary since you've imported the whole FirebaseFirestore module above.

Thanks for pointing it out, my Objective-C is pretty weak.

wilhuff · 2018-01-25T09:02:20Z

Firestore/Example/Tests/Integration/API/FIRTimestampAsArgumentTests.m

+
+#import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h"
+
+@interface FIRTimestampAsArgumentTests : FSTIntegrationTestCase


Mild nit: depending on how much these overlap with some of the other tests, as much as this arrangement makes it easier to write these tests, it could be harder to find them later when e.g. working on changing queries or user input validation.

To the extent we can do so, we should group these tests with the other tests with similar behavior for the other types. For example query limits are already explored in FIRQueryTests.

However it's also possible that there's just not much overlap, in which case, carry on.

I actually went back-and-forth on this once, so I don't have a strong stance here. Moved tests to existing files.

wilhuff · 2018-01-25T09:19:20Z

Firestore/Source/API/FIRDocumentSnapshot.m

-}
-
- (nullable id)valueForField:(id)field options:(FIRSnapshotOptions *)options {
+- (nullable FSTFieldValue*)fieldValueForKey:(id)key {


This is confusingly similar to valueForField:. Also I'm not sure this name really works: "keys" generally refer to document keys but fields/field paths refer to fields within a document. This is using key to mean field.

If you renamed this fieldValueForField to match the behavior of the argument then I think the functional confusion is probably too much.

Really the most important thing to share here is the bit that converts a field argument (which is an NSString or FIRFieldPath) to an FSTFieldPath. An alternative arrangement that might be clearer would be to pull that part out into a separate fieldPathForFieldArgument: helper and then have both valueForField and your new method call that while leaving them both to actually index into [[self.internalDocument data] valueForPath:] for themselves.

Even after renaming this could be overcome instead with comments, perhaps contrasting how these behave or what their return types are, etc.

I added this as a private helper method before valueForPath was added. Implemented your suggestion.

wilhuff · 2018-01-25T09:28:46Z

Firestore/Source/API/FIRDocumentSnapshot.m

  return result;
 }

+- (nullable FIRTimestamp*)getTimestamp:(id)key {


In general Objective-C naming is obtuse. Full guide here: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingMethods.html

Also, see Google-specific guidance here: http://google.github.io/styleguide/objcguide.html

Briefly, for a method that returns a value the label should start by describing that thing. Arguments get their own part of the label separated from the return value with a preposition. "get" as a prefix is almost never used.

Taken together with the feedback above that the argument is a field and not a key, this should likely be named something like timestampForField.

wilhuff · 2018-01-25T09:36:56Z

Firestore/Source/API/FIRTimestamp+Internal.h

-@interface FSTTimestamp : NSObject <NSCopying>
-
- (instancetype)init NS_UNAVAILABLE;
+@interface FIRTimestamp ()


I'm sort of surprised declaring this separately like this works and generates no warnings. It's not something we've commonly done.

I think I took inspiration from FIRFieldPath+Internal.h. It looks like I can't have a designated initializer anywhere apart from the class definition itself or in a class extension.

On a related note: can we use the same +Internal mechanism now that FIRTimestamp is to be a Firebase type?

EDIT: found examples in Firebase/Core showing that it's possible.

wilhuff · 2018-01-25T09:37:54Z

Firestore/Source/API/FIRTimestamp.mm

 */

-#import "Firestore/Source/Core/FSTTimestamp.h"
+#import "FIRTimestamp+Internal.h"


Only headers in the Public folder should be unqualified. All the rest should be fully qualified.

wilhuff · 2018-01-25T09:42:52Z

Firestore/Source/Public/FIRDocumentSnapshot.h

+ *
+ * @return The timestamp contained in the field or `nil` if the field doesn't exist.
+ */
+- (nullable FIRTimestamp *)getTimestamp:(id)key;


I'm surprised I didn't catch this looking at the API doc, but this is a highly non-idiomatic name as I commented above. The Objective-C-ish way of naming this would be timestampForField.

Note that objectForKeyedSubscript is an externally specified API that allows subscript notation to work against a document snapshot. We're using "key" there because that's what the API is documented as taking, but otherwise these values are "fields", not "keys".

wilhuff · 2018-01-25T09:43:07Z

Firestore/Source/Public/FIRTimestamp.h

@@ -0,0 +1,60 @@
+/*
+ * Copyright 2017 Google


Happy new year!

var-const

Two major changes left:

move FIRTimestamp to Core;
remove timestampForField, add new SnapshotOptions option.

Given the discussion on options today, should I add a new option to DocumentSnapshot as an overload of data/valueForField? With the existing SnapshotOptions, it's not clear how to extend it, and if we've decided to deprecate it, that doesn't seem worth the trouble.

var-const · 2018-01-29T23:15:20Z

Firestore/Example/Tests/Integration/API/FIRTimestampAsArgumentTests.m

@@ -0,0 +1,149 @@
+/*
+ * Copyright 2017 Google


var-const · 2018-01-29T23:18:09Z

Firestore/Example/Tests/Integration/API/FIRTimestampAsArgumentTests.m

+
+#import <XCTest/XCTest.h>
+
+#import "FirebaseFirestore/FIRTimestamp.h"


Thanks for pointing it out, my Objective-C is pretty weak.

var-const · 2018-01-29T23:19:34Z

Firestore/Example/Tests/Integration/API/FIRTimestampAsArgumentTests.m

+
+#import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h"
+
+@interface FIRTimestampAsArgumentTests : FSTIntegrationTestCase


I actually went back-and-forth on this once, so I don't have a strong stance here. Moved tests to existing files.

var-const · 2018-01-30T18:43:56Z

Firestore/Source/API/FIRDocumentSnapshot.m

-}
-
- (nullable id)valueForField:(id)field options:(FIRSnapshotOptions *)options {
+- (nullable FSTFieldValue*)fieldValueForKey:(id)key {


I added this as a private helper method before valueForPath was added. Implemented your suggestion.

var-const · 2018-01-30T18:57:25Z

Firestore/Source/Public/FIRDocumentSnapshot.h

+ *
+ * @return The timestamp contained in the field or `nil` if the field doesn't exist.
+ */
+- (nullable FIRTimestamp *)getTimestamp:(id)key;


var-const · 2018-01-30T18:57:38Z

Firestore/Source/Public/FIRTimestamp.h

@@ -0,0 +1,60 @@
+/*
+ * Copyright 2017 Google


var-const · 2018-01-30T19:31:33Z

Firestore/Source/API/FIRTimestamp+Internal.h

-@interface FSTTimestamp : NSObject <NSCopying>
-
- (instancetype)init NS_UNAVAILABLE;
+@interface FIRTimestamp ()


I think I took inspiration from FIRFieldPath+Internal.h. It looks like I can't have a designated initializer anywhere apart from the class definition itself or in a class extension.

On a related note: can we use the same +Internal mechanism now that FIRTimestamp is to be a Firebase type?

EDIT: found examples in Firebase/Core showing that it's possible.

var-const · 2018-02-08T22:47:34Z

Firestore/Source/API/FIRTimestamp.mm

 */

-#import "Firestore/Source/Core/FSTTimestamp.h"
+#import "FIRTimestamp+Internal.h"


var-const · 2018-02-08T22:47:55Z

Firestore/Source/API/FIRDocumentSnapshot.m

  return result;
 }

+- (nullable FIRTimestamp*)getTimestamp:(id)key {


…suite

var-const

PTAL.

Summary of changes:

FIRTimestamp is now in FirebaseCore, including the test. Removed usage of some Firestore helper functions;
as discussed today, reverted most Firestore-related changes. The only changes in Firestore are renamings, changes of import paths, and a test to check that DocumentSnapshot still returns NSDates (this test perhaps should also be split from this PR).

One thing I'd like feedback on in particular is the split of FIRTimestamp into public/private. I tried to imitate what other headers in Firestore were doing, but I don't know the full rationale. As it stands, most usages in Firestore require the private header, and I'm not sure whether it makes sense - perhaps the usages should be fixed, or some stuff moved from private to public header (it's mostly creation of timestamp with current date/time, and comparison).

var-const · 2018-02-09T23:59:33Z

Example/Core/Tests/FIRTimestampTest.m

+@interface FIRTimestampTest : XCTestCase
+@end
+
+NSDate *TestDate(int year, int month, int day, int hour, int minute, int second) {


I extracted this helper function from FSTHelpers.

var-const · 2018-02-10T00:02:43Z

Firebase/Core/Private/FIRTimestamp+Internal.h

@@ -0,0 +1,53 @@
+/*


I have a couple of questions regarding this file:

In general, what is the logic behind what should be kept in an internal header? I basically split all but the most basic functionality.

In the build process, how does the separation into public and private header work? I found out I could include <FirebaseCore/some_header_from_private_folder.h> anywhere in Firestore without any issue. Is there some special build step to make these headers available inside the SDK but unavailable for users, or are private headers always available, and it's just a convention that they shouldn't be used?

I have a couple of questions regarding this file:

In general, what is the logic behind what should be kept in an internal header? I basically split all but the most basic functionality.

The general rule is to reserve the public header for committed public APIs. Everything else goes in the private header.

Some classes within Firestore hide their initializers this way so that they must be constructed using other mechanisms. It's not universally required that initializers be hidden and indeed for plain old data types this makes the types unfriendly.

In the build process, how does the separation into public and private header work? I found out I could include <FirebaseCore/some_header_from_private_folder.h> anywhere in Firestore without any issue. Is there some special build step to make these headers available inside the SDK but unavailable for users, or are private headers always available, and it's just a convention that they shouldn't be used?

The private headers are available at build time within the SDK but aren't published as a part of the built framework. Source-only builds of the SDK have access to these, but source only builds can do whatever they want.

var-const · 2018-02-10T00:03:14Z

Firebase/Core/Public/FIRTimestamp.h

 NS_ASSUME_NONNULL_BEGIN

 /**
- * An FSTTimestamp represents an absolute time from the backend at up to nanosecond precision.


I'll rework this comment.

var-const · 2018-02-10T00:03:35Z

Firebase/Core/Public/FIRTimestamp.h


 #import <Foundation/Foundation.h>

 NS_ASSUME_NONNULL_BEGIN


Can NS_ASSUME_NONNULL be used in FirebaseCore?

var-const · 2018-02-10T00:05:14Z

Firestore/Example/Tests/Integration/API/FIRFieldsTests.mm

  [self awaitExpectations];
 }

+- (NSDictionary<NSString *, id> *)testDataWithTimestamp:(FIRTimestamp *)timestamp {


This test must break when we switch the default option later on.

var-const

Does our today's discussion about Timestamp code location on Android affect iOS? (i.e., should I move FIRTimestamp to Firestore/Source/Public?)

var-const · 2018-02-15T23:19:42Z

Example/Core/Tests/FIRTimestampTest.m

+@implementation FIRTimestampTest
+
+- (void)testFromDate {
+  // Very carefully construct an NSDate that won't lose precision with its milliseconds.


Done (with a very minor modification to the wording). I think this comment was originally copied from Android.

var-const · 2018-02-15T23:22:14Z

Firebase/Core/FIRTimestamp.m

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 Google
+ * Copyright 2018 Google


Done.

So the rationale seems to be:

if a file is modified OR moved, don't touch the copyright;

if the file is added, have a copyright with the current year;

if a file is modified AND moved, whether to update the copyright depends on whether the file was modified in any significant way.

Does the above look correct?

var-const · 2018-02-15T23:26:19Z

Firebase/Core/Public/FIRTimestamp.h

+ (instancetype)timestampWithSeconds:(int64_t)seconds nanoseconds:(int32_t)nanoseconds;

 /** Creates a new timestamp from the given date. */
 + (instancetype)timestampWithDate:(NSDate *)date;


@wilhuff, can/should I add some tests for Timestamp to Example/SwiftBuildTest/main.swift?

var-const · 2018-02-15T23:33:10Z

Firebase/Core/Private/FIRTimestamp+Internal.h

+ * @param seconds the number of seconds since epoch.
+ * @param nanoseconds the number of nanoseconds after the seconds.
+ */
+- (instancetype)initWithSeconds:(int64_t)seconds


Moved it to the public header.

I also moved timestamp factory function that returns a current timestamp to the public header, for consistency with Android (since we discussed today that `Timestamp#now' should exist on Android).

Let me know if you think compare should be moved to public as well. I'm less inclined towards ISOString - though it seems pretty cool, it doesn't currently exist on Android, so for consistency it would have to be implemented there as well.

var-const · 2018-02-15T23:41:50Z

Firebase/Core/Public/FIRTimestamp.h

 /**
- * An FSTTimestamp represents an absolute time from the backend at up to nanosecond precision.
- * An FSTTimestamp is represented in terms of UTC and does not have an associated timezone.
+ * An FIRTimestamp represents an absolute time from the backend at up to microsecond precision.


Does Objective C use the same Javadoc style as Android (i.e., should I modify the link into "@see <a.../>")?

wilhuff

Basically LGTM aside from source location and compare:.

Friday's discussion about Android's packaging difficulties does not apply as strongly here but it seems like we were agreed that Core is perhaps not the best place and that we needed to figure something else out.

I'm more inclined to get this committed already so this change isn't hanging outstanding forever. Let's commit this as part of Firestore now and then we can move it to its eventual home later.

wilhuff · 2018-02-20T05:20:11Z

Firebase/Core/FIRTimestamp.m

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 Google
+ * Copyright 2018 Google


If by "added" you mean the file is new, then yes. New files get the current year.

If you're searching for a standard consult git diff -C --find-copies-harder but if anyone could reasonably argue it's a copy of something else just leave it alone.

The standard in the law is "visually perceptible copies". Not "exact". You're not required to update the notice when making changes.

wilhuff · 2018-02-20T05:53:22Z

Firebase/Core/Private/FIRTimestamp+Internal.h

+ * @param seconds the number of seconds since epoch.
+ * @param nanoseconds the number of nanoseconds after the seconds.
+ */
+- (instancetype)initWithSeconds:(int64_t)seconds


Compare can be made public to parallel Android's Comparable<Timestamp>.

Don't bother with ISOString. The need is internal and making this consistently available is more painful than it's worth.

wilhuff · 2018-02-20T06:20:22Z

Firebase/Core/Public/FIRTimestamp.h

 /**
- * An FSTTimestamp represents an absolute time from the backend at up to nanosecond precision.
- * An FSTTimestamp is represented in terms of UTC and does not have an associated timezone.
+ * An FIRTimestamp represents an absolute time from the backend at up to microsecond precision.


Our Swift and Objective-C docs are published using Jazzy which uses a subset of the published standard keywords, documented here:

https://github.com/realm/jazzy#supported-keywords

AFAICT you can't insert the markup around the contents of the @see. It's just @see URL, like this:

firebase-ios-sdk/Firebase/Storage/Public/FIRStorageMetadata.h

Line 28 in a9f3f35

* @see https://cloud.google.com/storage/docs/json_api/v1/objects#resource

wilhuff · 2018-02-20T06:22:27Z

Firebase/Core/Public/FIRTimestamp.h

+ (instancetype)timestampWithSeconds:(int64_t)seconds nanoseconds:(int32_t)nanoseconds;

 /** Creates a new timestamp from the given date. */
 + (instancetype)timestampWithDate:(NSDate *)date;


It wouldn't hurt, but probably isn't worth it. We're about to add a bunch of actual tests for these public types via changes like #815 and there it's an actual test of behavior in Swift which is even more than our "does it build" demonstration in main.swift.

paulb777 · 2018-02-20T16:02:47Z

Once we go to source pods in Firebase 5.0, we'll have the infrastructure to make this an optional FirebaseCore subspec.

var-const

Moved timestamp files back to Firestore/Source/{Public,API}.

var-const · 2018-02-20T20:24:23Z

Firebase/Core/Private/FIRTimestamp+Internal.h

+ * @param seconds the number of seconds since epoch.
+ * @param nanoseconds the number of nanoseconds after the seconds.
+ */
+- (instancetype)initWithSeconds:(int64_t)seconds


Made compare public.

var-const · 2018-02-20T20:24:36Z

Firebase/Core/Public/FIRTimestamp.h

 /**
- * An FSTTimestamp represents an absolute time from the backend at up to nanosecond precision.
- * An FSTTimestamp is represented in terms of UTC and does not have an associated timezone.
+ * An FIRTimestamp represents an absolute time from the backend at up to microsecond precision.


wilhuff · 2018-02-20T21:22:35Z

Firestore/Example/Tests/Model/FSTFieldValueTests.mm

 #import <FirebaseFirestore/FIRGeoPoint.h>
-#import <FirebaseFirestore/FIRTimestamp.h>
 #import <XCTest/XCTest.h>
+#import "FIRTimestamp.h"


From tests these should be framework imports, like #import <FirebaseFirestore/FIRGeoPoint.h> above.

Changed in all test files.

wilhuff

LGTM

googlebot added the cla: yes label Jan 24, 2018

var-const requested a review from wilhuff January 24, 2018 20:26

wilhuff suggested changes Jan 25, 2018

View reviewed changes

var-const self-assigned this Jan 29, 2018

var-const added 11 commits January 30, 2018 14:39

review feedback 1

493a555

Merge branch 'master' into varconst-timestamp

da1f4c7

Fixes after merge, part 1

549ef59

fix merge error

7b211ab

nanoseconds wip

d4bf16e

fix tests for nanoseconds, start with truncation

6453f45

add a test for truncation, add comments re. truncation in tests

e0d6e01

Merge branch 'master' into varconst-timestamp

fac9034

.m -> .mm

4ca4b08

Fix include style

a29e546

run style.sh

0e44f1c

var-const commented Feb 8, 2018

View reviewed changes

var-const added 11 commits February 9, 2018 16:40

moving into shared folder wip

fbe2ff0

update imports of firtimestamp, exclude the test from firestore test …

a7a3187

…suite

Merge branch 'master' into varconst-timestamp

7f06221

update more import paths

5c6ae2d

Remove timestampForField

a24c7ed

fixing test wip

35b93c1

rename test to be more in line with existing tests in core/

18cd37e

test runs successfully

19d16ba

fixes to make all firestore tests pass

f8cc4d0

style.sh

a7edde5

remove Firestore accepting timestamps as parameters for now

ea8ced2

var-const commented Feb 10, 2018

View reviewed changes

fix failing test

571c41a

var-const commented Feb 16, 2018

View reviewed changes

wilhuff reviewed Feb 20, 2018

View reviewed changes

var-const added 9 commits February 20, 2018 12:41

Merge branch 'master' into varconst-timestamp

67bf53d

review feedback, 1

5401096

Move back

216b9ad

Add back

abbae33

style.sh

5e7eed4

fix import

4044ab2

remove reference to timestamp from Firebase project

0bb7320

move method

ba68d85

Merge branch 'master' into varconst-timestamp

032a2d3

var-const commented Feb 20, 2018

View reviewed changes

wilhuff reviewed Feb 20, 2018

View reviewed changes

var-const added 2 commits February 20, 2018 16:25

different includes in tests

e6e5e11

style.sh

e5c1b93

wilhuff approved these changes Feb 20, 2018

View reviewed changes

var-const merged commit 14ea068 into master Feb 20, 2018

zxu123 mentioned this pull request May 23, 2018

Firestore project: Import of API interfaces. zxu123/firebase-ios-sdk#3

Closed

paulb777 deleted the varconst-timestamp branch September 24, 2018 16:00

firebase locked and limited conversation to collaborators Nov 8, 2019


		#import <XCTest/XCTest.h>

		#import "FirebaseFirestore/FIRTimestamp.h"


		#import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h"

		@interface FIRTimestampAsArgumentTests : FSTIntegrationTestCase

Make FSTTimestamp into a public class: #698

Make FSTTimestamp into a public class: #698

Uh oh!

Conversation

var-const commented Jan 24, 2018

Uh oh!

var-const commented Jan 24, 2018

Uh oh!

wilhuff 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!

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!

var-const Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

var-const left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

var-const Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

var-const 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!

var-const Jan 30, 2018 •

edited

Loading

var-const left a comment •

edited

Loading

var-const Jan 30, 2018 •

edited

Loading

wilhuff left a comment •

edited

Loading