Skip to content
This repository was archived by the owner on Sep 15, 2025. It is now read-only.

Conversation

@crazytonyli
Copy link
Contributor

Description

@jkmassel raised a concern regarding changing the null_unspecified Objective-C properties to nullable, that since we may need a while to finish the translation from Objective-C to Swift, the apps will need to make lots of code changes (potentially repeatedly) to adopt the new API if we release new versions of WordPressKit for minor fixes. We agreed to keep the model properties backwards compatible with the original API, and create a long running branch for the Objective-C to Swift translation work.

There are three related PRs that were merged into trunk: #556, #557, and #558. They translated following models to Swift:

This PR reverts most of above model's properties back to Implicitly Unwrapped Optionals (as declared in their original Objective-C interface), except RemoteMenu, RemoteMenuItem, and RemoteMenuLocation since their original Objective-C interface declares their properties as all nullable.

Testing Details

I think we are all good as long as CI jobs are green.

  • Please check here if your pull request includes additional test coverage.
  • I have considered updating the version in the .podspec file.

@crazytonyli crazytonyli requested review from a team and jkmassel December 15, 2022 02:15
public var endpointPath: String!

public var postSubscription: RemoteReaderSiteInfoSubscriptionPost?
public var emailSubscription: RemoteReaderSiteInfoSubscriptionEmail?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The two properties above are kept as Optional since we know for sure that they are:

class func postSubscription(forSubscription subscription: NSDictionary) -> RemoteReaderSiteInfoSubscriptionPost? {
guard subscription.wp_isValidObject() else {
return nil
}
guard let deliveryMethod = subscription[SubscriptionDeliveryMethodsKey] as? [String: Any],
let method = deliveryMethod[DeliveryMethodNotificationKey] as? [String: Any]
else {
return nil
}
return RemoteReaderSiteInfoSubscriptionPost(dictionary: method)
}

class func emailSubscription(forSubscription subscription: NSDictionary) -> RemoteReaderSiteInfoSubscriptionEmail? {
guard subscription.wp_isValidObject() else {
return nil
}
guard let delieveryMethod = subscription[SubscriptionDeliveryMethodsKey] as? [String: Any],
let method = delieveryMethod[DeliveryMethodEmailKey] as? [String: Any]
else {
return nil
}
return RemoteReaderSiteInfoSubscriptionEmail(dictionary: method)
}

Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

One nit we should address, but then I think this should be good to go

### Breaking Changes

- Re-implement a few reader model types in Swift. [#556]
- Re-implement a few reader model types in Swift. [#556, #557, #558]
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken, this would be an un-breaking change, so we could probably move this out of the Breaking Changes section (and thus avoid cutting a new major version unnecessarily?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically speaking, I'd argue they are still breaking changes, even if we revert the two properties changes I mentioned above. The types are changed from Objective-C classes to Swift classes. And Swift class has its restrictions that Objective-C class don't have, like they cann't be inherited by an Objective-C class. I guess it's not likely that the apps have a subclass of these model types, but I'd say it's best to follow semantic version strictly, what do you think?

@crazytonyli
Copy link
Contributor Author

crazytonyli commented Dec 15, 2022

@jkmassel I'm going to merge this PR for now so that I can create the new dedicated Swift migration branch and change the open PRs to target the new branch. I'm happy to address further comments if you have more questions 🙂

@crazytonyli crazytonyli merged commit e0cc135 into trunk Dec 15, 2022
@crazytonyli crazytonyli deleted the model-property-backwards-compatiblity branch December 15, 2022 20:54
salimbraksa added a commit that referenced this pull request Dec 22, 2022
…ration

Fix a couple or backward compatibility issus missed in #562
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants