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

Conversation

@danielebogo
Copy link
Contributor

Description

Refs. #10277

Working on this PR I realized that the BlogService is not the right spot for the Jetpack activities. This because that service requires a dotComID as init value, which is nil for self-hosted sites. So instead of hardcoding an id equal to zero I moved that API to the JetpackServiceRemote.
This class was an objective-c class. I converted it in Swift and also fixed the test covering the whole class.

Testing Details

  • You can run the test: JetpackServiceRemoteTests
  • You can also checkout this branch

@danielebogo danielebogo requested review from a user and astralbodies April 10, 2019 14:35
@danielebogo danielebogo self-assigned this Apr 10, 2019
@ghost
Copy link

ghost commented Apr 10, 2019

@danielebogo thanks for pushing these changes forward, and for taking the initiative to transition some of our legacy code to Swift! I like where this is headed.

✅ I can confirm that the branch builds and tests pass.
✅ I also took your WPiOS branch for a spin, and confirmed that I was able to connect a self-hosted site, then install Jetpack successfully.

I have two questions:

  1. For the method JetpackServiceRemote.checkSiteHasJetpack(_:success:failure:), do you think this would benefit from an @objc annotation? Otherwise, it might be construed as a breaking change for Obj-C framework consumers.
  2. I agree with the rationale of moving JetpackServiceRemote.installJetpack(url:username:password:completion:) from BlogServiceRemoteREST to JetpackServiceRemote. Would you agree that this is a breaking change?

If we agree that this PR includes breaking changes for API consumers, we will need to revise the major version in WordPressKit.podspec and address the impact of that. Otherwise, this is looking good to me.

@danielebogo
Copy link
Contributor Author

danielebogo commented Apr 10, 2019

Thanks @stevebaranski for reviewing this and for your questions!

My first implementation had the objc annotation. I removed it because it's used only by the WordPressOrgXMLRPCValidator class, which is a Swift class. But I can add it again 😊

The Jetpack Remote Install task is still in development. The API has been introduced this week here. This is why I updated the minor version. But I completely agree with you. What would you suggest in this case?

@jtreanor @jkmassel what's the best way to upgrade the WPKit version to 4.0.0-beta.1? I'm thinking about the other frameworks that have WPKit as dependency with ~> 3.2.2

@jkmassel
Copy link
Contributor

If this is complete, feel free to tag this as 4.0.0-beta.1, it won't break existing builds.

However, you'll need to submit a PR for each dependency in order to have those take on the new version. Those should all have a ~> 4 requirement. In turn, that change will require a new version of each pod to be released. Once all are done, we'll do one big update in WP that brings all the pods to the latest version.

That should be the best way to ensure no breakage.

Does that all make sense?

@danielebogo
Copy link
Contributor Author

Thanks @jkmassel ! It makes perfectly sense!

@danielebogo
Copy link
Contributor Author

@stevebaranski I was thinking if this is going to be a major update, can we then omit the objc annotation?

@ghost
Copy link

ghost commented Apr 10, 2019

Hello again @danielebogo!

I was thinking if this is going to be a major update, can we then omit the objc annotation?

If we are changing the major version, then I think it's okay to omit the @objc annotation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Approved to merge, pending the dependency wrangling we discussed.
:shipit:

@danielebogo danielebogo merged commit c491899 into develop Apr 11, 2019
@danielebogo danielebogo deleted the fix/10277-move-jetpack-api branch April 11, 2019 11:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants