SPM Prep - Convert "WordPress JSON date" logic from Objective-C to Swift by mokagio · Pull Request #758 · wordpress-mobile/WordPressKit-iOS

mokagio · 2024-03-20T01:51:17Z

Description

This logic will need to live in the "Core API" package and the Jetpack/WordPress specific objects. It could have been extracted into a dedicated package, but for the sake of simplicity, it's been converted to Swift so it can live in "Core API".

Notice that:

I first added unit tests (smoke tests, more like) to ensure consistent behavior between implementation
The Objective-C dateWithWordPressComJSONString: method was translated by the compiler into NSDate(wordPressComJSONString:). In my conversion, I made it .with(wordPressComJSONString:). I think it would have been possible to define a convenience initializer to keep the API the same, but the approach I took seemed cleaner at the time. The method is only used internally to WordPressKit (see commit details) so, while it might count as a breaking change, I'm tempted to ignore it. Let me know if you think I'm being slack, or if it sounds like a reasonable compromise to move ahead without distractions.

Testing Details

See the new unit tests.

Next up

Basically, cherry-pick the work from #738 but in a neat and reviewable order:

Move files into Sources/ and Tests/
Introduce Bundle helper that differentiates between SPM and CocoaPods installations
Isolate core API objects in dedicated package
Then, new ground, define protocol abstraction to work around the Objective-C / Swift inter-op issue

See also, #756 which is related to the SPM work but independent from this one.

Please check here if your pull request includes additional test coverage. — N.A.
I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary. — N.A.

This resulted in a change in the Swift API. I think this is acceptable because the method was not used outside WordPressKit. See: - WordPressAuthenticator https://github.com/search?q=repo%3Awordpress-mobile%2FWordPressAuthenticator-iOS%20dateWithWordPressComJSONString&type=code - WordPress-iOS https://github.com/search?q=repo%3Awordpress-mobile%2FWordPress-iOS+dateWithWordPressComJSONString&type=code

It didn't work in Objective-C, but in Swift it's possible to do it and it makes more sense to have it implemented as `static let` so it's initialized only once.

mokagio · 2024-03-20T01:52:40Z

WordPressKit/NSDate+RFC3339.swift

+extension DateFormatter {
+
+    static let rfc3339Formatter: DateFormatter = {
+        let formatter = DateFormatter()
+        formatter.dateFormat = "yyyy'-'MM'-'dd'T'HH':'mm':'ssZ"
+        formatter.timeZone = NSTimeZone(forSecondsFromGMT: 0) as TimeZone
+        formatter.locale = NSLocale(localeIdentifier: "en_US_POSIX") as Locale
+        return formatter
+    }()
+}


It seemed neater to define this pre-configured DateFormatter in a DateFormatter extension, rather than NSDate.

mokagio · 2024-03-20T01:55:37Z

WordPressKit/PostServiceRemoteREST+Extended.swift

 private func makeParameters<T: Encodable>(from value: T) throws -> [String: AnyObject] {
    let encoder = JSONEncoder()
-    encoder.dateEncodingStrategy = .formatted(NSDate.rfc3339DateFormatter())
+    encoder.dateEncodingStrategy = .formatted(NSDate.rfc3339DateFormatter)


I forgot about the API breakage concerns when making this a static let. 🤦‍♂️

Turns out there is one use of this outside the library, in the WordPress iOS tests.

I'll update the code reintroducing the static func but marking it as deprecated. This way, we can remove it at the next "serious" breaking change occasion.

crazytonyli · 2024-03-20T21:57:12Z

Let me know if you think I'm being slack, or if it sounds like a reasonable compromise to move ahead without distractions.

My preference would be religiously following semantic versioning. There is no really difference in publishing 15.0 vs 14.2. Plus, when releasing a major version, you get to further clean up the codebase, i.e. moving extension NSDate to extension Date, removing NSDate.rfcXxxDateFormatter and using the one in DateFormatter instead, etc.

crazytonyli · 2024-03-20T22:02:51Z

WordPressKit/NSDate+RFC3339.swift

+
+extension DateFormatter {
+
+    static let rfc3339Formatter: DateFormatter = {


If you are going to make breaking changes, I guess we have an option of getting rid of the NSDate extension above and renaming this rfc3339Formatter to be something like wordPressComDateFormatter.

crazytonyli · 2024-03-20T22:06:58Z

WordPressKitTests/NSDate+RFC3339Tests.swift

+            NSDate(timeIntervalSince1970: 1_679_238_000).wordPressComJSONString(),
+            // Apparently, NSDateFormatter doesn't offer a way to specify Z vs +0000.
+            // This might go all the way back to the ISO 8601 and RFC 3339 specs overlap.
+            "2023-03-19T15:00:00+0000"


Have you tried to put a single quote ' around the Z in the format: "yyyy'-'MM'-'dd'T'HH':'mm':'ss'Z'"?

BTW, this is one of the reasons I think we should avoid using standard names in function names. It's pretty difficult to ensure conformance.

If we are being really strict here, this test case should fail, because the output is not in RFC3339 time format.

Thanks @crazytonyli !

I didn't noticed that 🤦‍♂️

I tried locally and it does the job at returning 2023-03-19T15:00:00Z as I originally expected.

But ☝️ other tests that deal with API and dates fail. I think maybe we should leave it like this for the moment, and update / investigate later on. After all, the format the code uses right now is the "original" one.

What do you think?

mokagio · 2024-03-20T22:07:23Z

My preference would be religiously following semantic versioning

Good point. It also makes the decision making simpler "is this a breaking change (regardless of whether the API is public but with no consumers)" "yes?" "new major version."

Boom 🚀

See 71fa951

The breaking change is actually in having removed `NSDate.rfc3339Formatter`, which is no longer necessary.

mokagio · 2024-03-20T23:38:01Z

@crazytonyli thanks again for the suggestions. I implemented them in #759

crazytonyli · 2024-03-21T20:09:03Z

WordPressKit.podspec

 Pod::Spec.new do |s|
  s.name          = 'WordPressKit'
-  s.version       = '14.1.0'
+  s.version       = '15.0.0-beta.1'


Usually we make version change in a dedicated PR along side with moving changelog content. Do you intend to make a release once this PR is merged?

Oh, right. I just wanted to put a line in the sand for the breaking change.

mokagio added 6 commits March 19, 2024 21:12

Add tests for the NSDate extension implementing RFC3339

360a0a2

Convert -(NSString *)WordPressComJSONString method to Swift

50916f6

Convert rfc3339DateFormatter method to Swift

51f0616

Delete unused NSDate+WordPressJSON Objective-C file

b0e6dcf

Make new Swift rfc3339DateFormatter a static let instead of func

b0ed585

It didn't work in Objective-C, but in Swift it's possible to do it and it makes more sense to have it implemented as `static let` so it's initialized only once.

mokagio enabled auto-merge March 20, 2024 01:51

mokagio requested a review from crazytonyli March 20, 2024 01:51

mokagio commented Mar 20, 2024

View reviewed changes

Restore rfc3339DateFormatter() method for backwards compatibility

5686c93

crazytonyli reviewed Mar 20, 2024

View reviewed changes

mokagio added 4 commits March 21, 2024 09:14

Bump version to 15.0.0-beta.1 because of breaking change in NSDate API

31292ae

See 71fa951

Breaking change – Remove deprecated rfc3339DateFormatter()

f3bc0a0

Extract DateFormatter RFC-3339 to file and rename to wordPressCom

ca4b342

Breaking change – Move NSDate WordPress.com implementation to Date

e4f8fba

The breaking change is actually in having removed `NSDate.rfc3339Formatter`, which is no longer necessary.

mokagio mentioned this pull request Mar 20, 2024

Modernize NSDate and Date implementation introducing breaking changes #759

Merged

2 tasks

mokagio mentioned this pull request Mar 21, 2024

SPM Prep – Add @protocol defining REST GET and POST methods #760

Merged

2 tasks

mokagio and others added 2 commits March 21, 2024 17:57

Breaking change – Modernize NSDate and Date WP.com extensions (#759)

2a90451

Add CHANGELOG entry for reworked WP.com NSDate conversion

4ab30ee

mokagio requested a review from crazytonyli March 21, 2024 07:00

crazytonyli approved these changes Mar 21, 2024

View reviewed changes

mokagio merged commit 118fd9a into trunk Mar 21, 2024

mokagio deleted the mokagio/rfc-3339 branch March 21, 2024 20:09

This was referenced Mar 22, 2024

SPM Prep – Use WordPressComRESTAPIInterfacing in the Objective-C consumers #761

Merged

SPM Prep – Add multipart POST method to WordPressComRESTAPIInterfacing #765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPM Prep - Convert "WordPress JSON date" logic from Objective-C to Swift#758

SPM Prep - Convert "WordPress JSON date" logic from Objective-C to Swift#758
mokagio merged 13 commits intotrunkfrom
mokagio/rfc-3339

mokagio commented Mar 20, 2024 •

edited

Loading

Uh oh!

mokagio Mar 20, 2024

Uh oh!

mokagio Mar 20, 2024

Uh oh!

crazytonyli commented Mar 20, 2024

Uh oh!

crazytonyli Mar 20, 2024

Uh oh!

crazytonyli Mar 20, 2024

Uh oh!

crazytonyli Mar 20, 2024

Uh oh!

crazytonyli Mar 20, 2024

Uh oh!

mokagio Mar 20, 2024

Uh oh!

mokagio commented Mar 20, 2024

Uh oh!

mokagio commented Mar 20, 2024

Uh oh!

crazytonyli Mar 21, 2024

Uh oh!

mokagio Mar 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		extension DateFormatter {

		static let rfc3339Formatter: DateFormatter = {

Conversation

mokagio commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing Details

Next up

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazytonyli commented Mar 20, 2024

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!

mokagio commented Mar 20, 2024

Uh oh!

mokagio commented Mar 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mokagio commented Mar 20, 2024 •

edited

Loading