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

SPM Prep - Isolate API version enum and convert to modern Objective-C #778

Merged
merged 5 commits into from
Apr 5, 2024

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Apr 4, 2024

Description

The end goal is to extract / duplicate the pathForEndpoint:withVersion: logic so that it can be used outside of ServiceRemoteWordPressComRESTApiVersion. The reason for that is that while all the sync calls can be used across Swift and Objective-C via #777, the async one cannot be used in Objective-C and therefore need Swift-only types that do not depend on WordPressComRESTAPIInterfacing

Testing Details

No behavior change here. If the unit tests compile, we should be good.


  • 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.

let path = self.path(forEndpoint: "me/social-login/connect", withVersion: ._1_1)
let path = self.path(forEndpoint: "me/social-login/connect", with: ._1_1)
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 compiler changed this, which was a bit annoying because it made the diff quite noisy.

I guess the compiler is happy with just "with" when the type on the other side is "strong", i.e. a typedef NS_ENUM that converts neatly to enum in Swift.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid this name change by adding a NS_SWIFT_NAME(path(forEndpoint:withVersion:)) to the function in the ObjC header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! Much better diff, thanks for the tip @crazytonyli

I think the Swift API is better this way, too. with: ._1_1 was not as clear as withVersion: ._1_1.

@mokagio mokagio requested a review from crazytonyli April 4, 2024 10:46
Base automatically changed from mokagio/convert-all-wordpresscomrestapi-swift to trunk April 4, 2024 22:12
@mokagio mokagio changed the title Isolate API version enum and convert to modern Objective-C SPM Prep - Isolate API version enum and convert to modern Objective-C Apr 4, 2024
@mokagio mokagio force-pushed the mokagio/rename-apiversion-enum branch from c437048 to 25efc1f Compare April 4, 2024 23:44
@mokagio mokagio enabled auto-merge April 4, 2024 23:46
@mokagio mokagio force-pushed the mokagio/rename-apiversion-enum branch from 25efc1f to 071a046 Compare April 4, 2024 23:47
@mokagio mokagio force-pushed the mokagio/rename-apiversion-enum branch from 071a046 to ee3d90b Compare April 5, 2024 05:37
@mokagio
Copy link
Contributor Author

mokagio commented Apr 5, 2024

Force pushed to resolve merge conflict without having to add merge commit.

@mokagio mokagio merged commit d149306 into trunk Apr 5, 2024
9 checks passed
@mokagio mokagio deleted the mokagio/rename-apiversion-enum branch April 5, 2024 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants