SPM Prep – Define new CoreAPI package #782

mokagio · 2024-04-08T08:48:48Z

Description

Moves some of the lower level Swift files that implement API requests into Sources/CoreAPI with matching tests.

Getting the tests to compile in this setup required some additional work in terms of conditional imports and an SPM-aware Bundle loader.

Notice that the package exists but is not used anywhere. I just wanted to lock this in, so to speak, to build on top of it later on.

Testing Details

See CI. Although, it only shows that no regression has been introduced by this setup.

To test the SPM setup alone, one needs to:

Delete the .xc- workspace and project folder

Test via xcodebuild, for example with a lane like this one:

WordPressKit-iOS/fastlane/Fastfile

Lines 10 to 29 in 5c002d7

    
           lane :package_build do 
        
             scheme = 'WordPressKit-Package' 
        
             # We need to explicitly step the derived data folder, otherwise we'll get errors like: 
        
             # > Cannot find .xcresult in derived data which is needed to determine test results. 
        
             derived_data_path = File.join(__dir__, '.build', 'derived-data') 
        
             xcodebuild( 
        
               scheme:, 
        
               xcargs: "-resolvePackageDependencies #{SWIFTLINT_PLUGIN_VALIDATION_BYPASS_XCARGS} -derivedDataPath #{derived_data_path}" 
        
             ) 
        
             run_tests( 
        
               package_path: '.', 
        
               scheme:, 
        
               device: 'iPhone 15', 
        
               prelaunch_simulator: true, 
        
               xcargs: SWIFTLINT_PLUGIN_VALIDATION_BYPASS_XCARGS, 
        
               derived_data_path: 
        
             )

OR

Open the project in Xcode via Package.swift (open -a /Applications/Xcode.app . should do it)
Run the tests

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.

dangermattic · 2024-04-08T08:49:45Z

	2 Warnings
⚠️	`Package.swift` was changed without updating its corresponding `Package.resolved`. Please resolve the Swift packages in Xcode.
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

mokagio · 2024-04-11T23:29:52Z

Tests/CoreAPITests/WordPressOrgXMLRPCApiTests.swift

+                // When building for SPM, the compiler doesn't generate the domain constant.
+#if SWIFT_PACKAGE
+                XCTAssertEqual(error.domain, "CoreAPI.WordPressOrgXMLRPCApiError")
+#else
                XCTAssertEqual(error.domain, WordPressOrgXMLRPCApiErrorDomain)
+#endif


@crazytonyli what do you think of this?

I worry it might backfire in the real world if people where to do a comparison on the domain with a String literal value.

Maybe it would be safer to explicitly define the domain in a let and update the code that inits WordPressOrgXMLRPCApiError to set it?

My preference would be going with the safer option, which is avoiding breaking changes. You can implement CustomNSError to set a custom domain, which means we shouldn't need to change this assertion here.

Partially addressed in 3f57895.

However, the tests still fail when build for SPM. I believe that has to do with the XML RPC returning WordPressOrgXMLRPCApiError or WordPressOrgXMLRPCApiFault.

I'll have to look into the latter as well, I guess.

I found the source of the issue.

If one removes the @objc annotation, then the CustomNSError conversion code is used in its entirety. Otherwise, with the error enum declared as @objc, it seems like only the part that computes userInfo is called.

I verified this with an ad hoc package. I might put it online later. I'm unsure how to move forward in the meantime, because we do need the @objc visibility for WordPressOrgXMLRPCApiError. I'll keep thinking about it...

Move my WIP work to #790 and rebased it off this branch.

mokagio · 2024-04-11T23:32:41Z

Sources/CoreAPI/WordPressComOAuthClient.swift

-                WPKitLogVerbose("Received OAuth2 response: \(self.cleanedUpResponseForLogging(responseObject as AnyObject? ?? "nil" as AnyObject))")
+                // WPKitLogVerbose("Received OAuth2 response: \(self.cleanedUpResponseForLogging(responseObject as AnyObject? ?? "nil" as AnyObject))")


Might need to introduce a new abstraction layer for logging, too.

Or, is logging at this level useful? Can we simply get rid of it altogether?

I opened an issue about it #787

crazytonyli · 2024-04-12T01:27:47Z

Sources/CoreAPI/WordPressComRestApiErrorDomain.swift

+let WordPressComRestApiErrorDomain = "WordPressKit.WordPressComRestApiError"
+
+// WordPressComRestApiErrorDomain is accessible only in Swift and, since it's a global, cannot be made @objc.
+// As a workaround, here is a builder to init NSError with that domain and a method to check the domain.


An alternative could be defining the same constant again in Objective-C and making it unavailable to Swift. That way you won't need to change those Objective-C files.

See 6fba3a1

crazytonyli · 2024-04-12T01:33:12Z

Tests/CoreAPITests/WordPressOrgXMLRPCApiTests.swift

+                // When building for SPM, the compiler doesn't generate the domain constant.
+#if SWIFT_PACKAGE
+                XCTAssertEqual(error.domain, "CoreAPI.WordPressOrgXMLRPCApiError")
+#else
                XCTAssertEqual(error.domain, WordPressOrgXMLRPCApiErrorDomain)
+#endif


My preference would be going with the safer option, which is avoiding breaking changes. You can implement CustomNSError to set a custom domain, which means we shouldn't need to change this assertion here.

@crazytonyli

This DRYs the definition, makes the constant available to both Objective-C and Swift, and removes the need for custom builder and checker. See @crazytonyli's suggestion from #782 (comment)

mokagio commented Apr 11, 2024

View reviewed changes

mokagio marked this pull request as ready for review April 11, 2024 23:34

mokagio enabled auto-merge April 11, 2024 23:47

crazytonyli reviewed Apr 12, 2024

View reviewed changes

mokagio added 10 commits April 13, 2024 07:49

Define new CoreAPI package — Currently only WordPressAPIError in it

0412d8d

Move a couple more files

70b798c

Update SwiftLint config to ignore .build from SPM

61a40e1

Move all remaining source files

b87c4c0

Comment all WPKitLog* usages in CoreAPI to get to build

576a820

Move remaining tests to CoreAPITests/

dac09cd

Move fixtures to CoreAPITests

dbd8322

Move ServiceRemoteWordPressComREST to new BasicBlogAPIObjc/

281cf4b

Extract logic to compute REST path with version

d1c738c

Move WordPressComRESTAPIVersionedPathBuilder to APIInterface/

46e3a5f

mokagio force-pushed the mokagio/coreapi-package branch from 79384d2 to 46e3a5f Compare April 12, 2024 22:13

Add SPM-ok imports in WordPressComRESTAPIVersionedPathBuilder

7198442

mokagio force-pushed the mokagio/coreapi-package branch 2 times, most recently from 22dcdd8 to 7198442 Compare April 15, 2024 05:59

mokagio mentioned this pull request Apr 15, 2024

Make WordPressOrgXMLRPCApiError conform to CustomNSError #790

Open

2 tasks

Move WordPressComRestApiErrorDomain to APIInterface

6fba3a1

This DRYs the definition, makes the constant available to both Objective-C and Swift, and removes the need for custom builder and checker. See @crazytonyli's suggestion from #782 (comment)

crazytonyli approved these changes Apr 16, 2024

View reviewed changes

mokagio merged commit 7b624cd into trunk Apr 16, 2024
9 checks passed

mokagio deleted the mokagio/coreapi-package branch April 16, 2024 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPM Prep – Define new CoreAPI package #782

SPM Prep – Define new CoreAPI package #782

mokagio commented Apr 8, 2024 •

edited

Loading

dangermattic commented Apr 8, 2024

mokagio Apr 11, 2024

crazytonyli Apr 12, 2024

mokagio Apr 13, 2024 •

edited

Loading

mokagio Apr 15, 2024

mokagio Apr 15, 2024

mokagio Apr 11, 2024

mokagio Apr 11, 2024

crazytonyli Apr 12, 2024

mokagio Apr 15, 2024

crazytonyli Apr 12, 2024

	lane :package_build do
	scheme = 'WordPressKit-Package'

	# We need to explicitly step the derived data folder, otherwise we'll get errors like:
	# > Cannot find .xcresult in derived data which is needed to determine test results.
	derived_data_path = File.join(__dir__, '.build', 'derived-data')

	xcodebuild(
	scheme:,
	xcargs: "-resolvePackageDependencies #{SWIFTLINT_PLUGIN_VALIDATION_BYPASS_XCARGS} -derivedDataPath #{derived_data_path}"
	)

	run_tests(
	package_path: '.',
	scheme:,
	device: 'iPhone 15',
	prelaunch_simulator: true,
	xcargs: SWIFTLINT_PLUGIN_VALIDATION_BYPASS_XCARGS,
	derived_data_path:
	)

		WPKitLogVerbose("Received OAuth2 response: \(self.cleanedUpResponseForLogging(responseObject as AnyObject? ?? "nil" as AnyObject))")
		// WPKitLogVerbose("Received OAuth2 response: \(self.cleanedUpResponseForLogging(responseObject as AnyObject? ?? "nil" as AnyObject))")

SPM Prep – Define new CoreAPI package #782

SPM Prep – Define new CoreAPI package #782

Conversation

mokagio commented Apr 8, 2024 • edited Loading

Description

Testing Details

dangermattic commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Apr 8, 2024 •

edited

Loading

mokagio Apr 13, 2024 •

edited

Loading