Skip to content

Conversation

@crazytonyli
Copy link
Contributor

@crazytonyli crazytonyli commented Jun 17, 2024

They are still used as a CocoaPods pod.

You can ignore the first commit, because it's just copying the source code from the trunk branches, without any code changes. Only library and test code are moved over.

A quick smoke test should be sufficient: login, viewing posts, reader, notifications, etc.

Next step

I'll look into creating Xcode targets for them, so that we can run their unit tests locally. Also, that'll get us a lot closer to SPM.

@crazytonyli crazytonyli requested a review from jkmassel June 17, 2024 22:18
@crazytonyli crazytonyli self-assigned this Jun 17, 2024
@crazytonyli crazytonyli added the Tooling Build, Release, and Validation Tools label Jun 17, 2024
@crazytonyli crazytonyli added this to the 25.2 milestone Jun 17, 2024
@crazytonyli
Copy link
Contributor Author

[...]/gems/octokit-8.1.0/lib/octokit/response/raise_error.rb:14:in `on_complete': GET https://api.github.com/repos/wordpress-mobile/WordPress-iOS/pulls/23366: 406 - Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead. (Octokit::NotAcceptable)

@iangmaia Dangermattic check failed because this PR diff is huge...

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 17, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23366-b781954
Version25.1
Bundle IDorg.wordpress.alpha
Commitb781954
App Center BuildWPiOS - One-Offs #10206
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 17, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23366-b781954
Version25.1
Bundle IDcom.jetpack.alpha
Commitb781954
App Center Buildjetpack-installable-builds #9255
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@kean
Copy link
Contributor

kean commented Jun 19, 2024

Hey, is it based on Gio's recent proposal? I've also been advocating in favor of monorepos then, but I'm just curious if there was a decision about how the client that are not WP-iOS will consume it. If you have to download this repo, a shallow clone is 100 MB, which is a bit steep, but maybe not too bad if it's just Woo.

kean@Alexs-MacBook-Pro Desktop % time git clone --depth=1 https://github.com/wordpress-mobile/WordPress-iOS
Cloning into 'WordPress-iOS'...
remote: Enumerating objects: 7933, done.
remote: Counting objects: 100% (7933/7933), done.
remote: Compressing objects: 100% (5785/5785), done.
remote: Total 7933 (delta 1847), reused 5286 (delta 1372), pack-reused 0
Receiving objects: 100% (7933/7933), 93.66 MiB | 28.25 MiB/s, done.
Resolving deltas: 100% (1847/1847), done.
Updating files: 100% (6524/6524), done.
git clone --depth=1 https://github.com/wordpress-mobile/WordPress-iOS  1.67s user 1.13s system 41% cpu 6.726 total

@crazytonyli
Copy link
Contributor Author

is it based on Gio's recent proposal?

It's per request of @jkmassel .

The motivation is mainly around faster iteration, not so much about monorepo though. Woo doesn't actually use WPKit. Woo depends on WPKit simply because WPKit is a dependency of WPAuthenticator, which Woo does use.

Copy link
Contributor

@kean kean left a comment

Choose a reason for hiding this comment

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

LGTM.

  • I was able to build and run it
  • The code contains the latest change from WordPressKit-iOS's trunk
  • Smoke tested the login flow

@crazytonyli crazytonyli enabled auto-merge June 20, 2024 21:22
@crazytonyli crazytonyli disabled auto-merge June 20, 2024 21:44
@crazytonyli
Copy link
Contributor Author

SonarCloud reported 13 issues: 7 high and 6 low priority. All 7 high priority issues are in unit tests. I'm going to ignore the issues for now.

Danger check failed due to large diff in this PR. I'll also ignore that.

I'll merge this PR as all the rest checks passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Tooling Build, Release, and Validation Tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants