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

Move Timestamp class from Firestore to Firebase Core #13221

Merged
merged 8 commits into from
Jul 12, 2024

Conversation

cherylEnkidu
Copy link
Contributor

@cherylEnkidu cherylEnkidu commented Jul 3, 2024

Move Timestamp class into FirebaseCore. FirebaseFirestore.Timestamp was changed to FirebaseCore.Timestamp.

Copy link
Contributor

github-actions bot commented Jul 3, 2024

Apple API Diff Report

Commit: ae746d4
Last updated: Fri Jul 12 12:29 PDT 2024
View workflow logs & download artifacts


[BUILD ERROR] FirebaseFirestore


FirebaseCore

Classes

[ADDED] FIRTimestamp
[ADDED] FIRTimestamp
Swift:
+  class Timestamp : NSObject , NSCopying
+    init ( seconds : Int64 , nanoseconds : Int32 )
+    convenience init ( date : Date )
+    convenience init ()
+    func dateValue () -> Date
+    func compare ( _ other : Timestamp ) -> ComparisonResult
+    var seconds : Int64 { get }
+    var nanoseconds : Int32 { get }
Objective-C:
+  @interface FIRTimestamp : NSObject < NSCopying >
+    - ( nonnull instancetype ) initWithSeconds :( int64_t ) seconds nanoseconds :( int32_t ) nanoseconds ;
+    + ( nonnull instancetype ) timestampWithSeconds :( int64_t ) seconds nanoseconds :( int32_t ) nanoseconds ;
+    + ( nonnull instancetype ) timestampWithDate :( nonnull NSDate * ) date ;
+    + ( nonnull instancetype ) timestamp ;
+    - ( nonnull NSDate * ) dateValue ;
+    - ( NSComparisonResult ) compare :( nonnull FIRTimestamp * ) other ;
+    @property ( nonatomic , readonly ) int64_t seconds ;
+    @property ( nonatomic , readonly ) int32_t nanoseconds ;

[BUILD ERROR] FirebaseFirestoreInternal


@google-oss-bot
Copy link

google-oss-bot commented Jul 3, 2024

Coverage Report 1

Affected Products

  • FirebaseFirestore-iOS-FirebaseFirestoreInternal.framework

    Overall coverage changed from 88.21% (eca84fd) to 88.17% (056560a) by -0.04%.

    FilenameBase (eca84fd)Merge (056560a)Diff
    exception.cc84.21%23.68%-60.53%
    leveldb_key.cc98.63%99.02%+0.39%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/gML4LaVlPF.html

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Good to merge to main after July 8th.

It can merge to the release-11.0 branch before that.

@cherylEnkidu
Copy link
Contributor Author

Good to merge to main after July 8th.

It can merge to the release-11.0 branch before that.

Hi @paulb777 , just want to double check, I can merge to main branch after July 8th?

@paulb777
Copy link
Member

paulb777 commented Jul 8, 2024

Yes - 10.29.0 is releasing and the release-11.0 branch has merged to main.

It looks like there are some conflicts. Let icore know if you have any questions about them.

@paulb777 paulb777 added this to the Firebase 11 - M151 milestone Jul 10, 2024
@cherylEnkidu cherylEnkidu merged commit 3de44b7 into main Jul 12, 2024
56 checks passed
@cherylEnkidu cherylEnkidu deleted the cheryllin/moveTimestamp branch July 12, 2024 20:19
Comment on lines +20 to +21
- [changed] Move `Timestamp` class into `FirebaseCore`. `FirebaseFirestore.Timestamp`
was changed to `FirebaseCore.Timestamp`. (#13221)
Copy link
Member

@ncooke3 ncooke3 Jul 16, 2024

Choose a reason for hiding this comment

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

FirebaseFirestore.Timestamp was changed to FirebaseCore.Timestamp.

@paulb777 – This is a small break, but I believe we could add:

@_exported import class FirebaseCore.Timestamp

to selectively import and re-export that class so it stills appears under the FirebaseFirestore namespace (like what was done with Swift extensions post-consolidation). This would make the move non-breaking.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM. It should minimize the break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I create a new PR to add this line in FirebaseFirestoreSwift.swift or there is a pr working in progress?

Copy link
Member

Choose a reason for hiding this comment

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

@cherylEnkidu, sure! I haven't started anything. For all the files in this PR where you needed to add import FirebaseCore, I think you should be able to just replace those with @_exported import class FirebaseCore.Timestamp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I will send you a PR shortly.

@firebase firebase locked and limited conversation to collaborators Aug 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants