refactor: Port MPListenerController to Swift #320
Conversation
|
|
||
| import Foundation | ||
|
|
||
| @objc public enum MPEndpoint: Int { |
There was a problem hiding this comment.
Let's move these 2 enums and the MPListenerProtocol into a dedicated MPListenerProtocol.swift file
| * @param sdkListener An instance of a class that implements the MPListenerProtocol | ||
| */ | ||
| @objc public func addSdkListener(_ sdkListener: any MPListenerProtocol) { | ||
| self.sdkListeners.append(sdkListener) |
There was a problem hiding this comment.
| self.sdkListeners.append(sdkListener) | |
| sdkListeners.append(sdkListener) |
Per style guide, let's get rid of these explicit self. calls in
| * @param sdkListener An instance of a class that implements the MPListenerProtocol | ||
| */ | ||
| @objc public func removeSdkListener(_ sdkListener: any MPListenerProtocol) { | ||
| self.sdkListeners = self.sdkListeners.filter { sdkListener !== $0 } |
There was a problem hiding this comment.
| self.sdkListeners = self.sdkListeners.filter { sdkListener !== $0 } | |
| sdkListeners = sdkListeners.filter { sdkListener !== $0 } |
There was a problem hiding this comment.
Another thing, this actually has different behavior than the original Obj-C code due to the use of the !== identity equality operator. The original removeObject: call in Obj-C will use the isEqual: method to compare objects, which is essentially equivalent to == in Swift, but you're using the === form here (or well the negative !== form vs !=) which checks for address equality, i.e. exact same object.
In this case it may not matter, but was this intentional?
| case identityLogin = 0, identityLogout, identityIdentify, identityModify, events, config, alias | ||
| } | ||
| @objc public enum MPDatabaseTable: Int { | ||
| case attributes = 0, breadcrumbs, messages, reporting, sessions, uploads, unknown |
There was a problem hiding this comment.
This style is fine for now, but let's discuss later if we want to use this style or enforce a more classic "every item on it's own line" style.
|
|
||
| @objc public func onAPICalled(_ apiName: Selector, parameter1: NSObject?, parameter2: NSObject?, parameter3: NSObject?) { | ||
| for delegate in self.sdkListeners { | ||
| let stackTrace = [Thread.callStackSymbols] |
There was a problem hiding this comment.
| let stackTrace = [Thread.callStackSymbols] | |
| let stackTrace = Thread.callStackSymbols |
This has different behavior from the original Obj-C. The Thread.callStackSymbols method returns an array ([String]) already, so this is putting an array inside another array.
|
|
||
| @objc public func onAPICalled(_ apiName: Selector, parameter1: NSObject?, parameter2: NSObject?) { | ||
| for delegate in self.sdkListeners { | ||
| let stackTrace = [Thread.callStackSymbols] |
There was a problem hiding this comment.
| let stackTrace = [Thread.callStackSymbols] | |
| let stackTrace = Thread.callStackSymbols |
| if parameter1 != nil { | ||
| parameters.append(parameter1!) |
There was a problem hiding this comment.
| if parameter1 != nil { | |
| parameters.append(parameter1!) | |
| if let parameter1 = parameter1 { | |
| parameters.append(parameter1) |
All of these nil checks should be replaced with if let and all uses of the ! operator removed.
| * @param isExternal true, if the call originated from outside of the SDK | ||
| * @param objects is the arguments sent to this api, such as the MPEvent in logEvent | ||
| */ | ||
| @objc optional func onAPICalled(_ apiName: Any!, stackTrace: Any!, isExternal: Any!, objects: Any!) |
There was a problem hiding this comment.
I assume the !'s here are to keep the same public interface as before?
| } | ||
|
|
||
| @objc public func onAPICalled(_ apiName: Selector, parameter1: NSObject?, parameter2: NSObject?, parameter3: NSObject?) { | ||
| for delegate in self.sdkListeners { |
There was a problem hiding this comment.
| for delegate in self.sdkListeners { | |
| for delegate in sdkListeners { |
All of these explicit calls to self should be removed per the style guide
|
Also I notice this doesn't seem to currently be used in any tests? We should add some before merging this. |
Summary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)