-
Notifications
You must be signed in to change notification settings - Fork 68
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
refactor: Update NSDictionary+CaseInsensitive to Swift #326
base: development
Are you sure you want to change the base?
refactor: Update NSDictionary+CaseInsensitive to Swift #326
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the MPKitContainerTests testForwardAppsFlyerEvent
test is failing on this branch due to these changes. I checked out the development branch and confirmed it passes there.
Once that test is fixed and new dedicated tests added for these methods, I can re-review and make any final suggestions to simplify the implementation.
|
||
@objc func transformValuesToString() -> [String : Any] { | ||
let transformedDictionary: [String : Any] = self.reduce(into: [:]) { result, element in | ||
let key = element.key as! String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will crash any time element.key
is not a String. Since this is inside a reduce
closure, you can use a guard let
statement here and it will return from the closure, not the whole function.
Something like this should work:
let key = element.key as! String | |
guard let key = element.key as? String else { return } |
|
||
public func transformValuesToString() -> [String : Any] { | ||
let transformedDictionary: [String : Any] = self.reduce(into: [:]) { result, element in | ||
let key = element.key as! String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
let key = element.key as! String | |
guard let key = element.key as? String else { return } |
var localKey = key | ||
|
||
self.allKeys.forEach { keyValue in | ||
if let stringKey = keyValue as? String, stringKey.caseInsensitiveCompare(key) == .orderedSame { | ||
localKey = stringKey | ||
} | ||
} | ||
return localKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has slightly different behavior than the original Obj-C code. In the original, it stops searching after finding the correct key, but in this code it will continue. It's not possible to use break here as it it's an actual for loop, and if you use return it will return from the closure, but I believe it will continue to loop. Instead this can be written as a for in loop:
var localKey = key | |
self.allKeys.forEach { keyValue in | |
if let stringKey = keyValue as? String, stringKey.caseInsensitiveCompare(key) == .orderedSame { | |
localKey = stringKey | |
} | |
} | |
return localKey | |
for aKey in allKeys { | |
if let stringKey = aKey as? String, stringKey.caseInsensitiveCompare(key) == .orderedSame { | |
return stringKey | |
} | |
} | |
return key |
var value : Any? | ||
|
||
self.allKeys.forEach { keyValue in | ||
if let stringKey = keyValue as? String, stringKey.caseInsensitiveCompare(key) == .orderedSame { | ||
value = self[stringKey] | ||
} | ||
} | ||
|
||
return value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Also caqn be slightly simplified since with a for in
loop you can get the key and value directly:
var value : Any? | |
self.allKeys.forEach { keyValue in | |
if let stringKey = keyValue as? String, stringKey.caseInsensitiveCompare(key) == .orderedSame { | |
value = self[stringKey] | |
} | |
} | |
return value | |
for (aKey, value) in self { | |
if let stringKey = aKey as? String, stringKey.caseInsensitiveCompare(key) == .orderedSame { | |
return value | |
} | |
} | |
return nil |
return value | ||
} | ||
|
||
@objc func transformValuesToString() -> [String : Any] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method really needs a dedicate unit test written in Objective-C that passes in a dictionary of all of the supported types from all the ifs and confirms the output strings. First make sure it passes against the original Obj-C implementation then against this one.
} | ||
|
||
@objc func transformValuesToString() -> [String : Any] { | ||
let transformedDictionary: [String : Any] = self.reduce(into: [:]) { result, element in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double check that the use of the reduce method here doesn't bridge the NSDictionary to a Swift dict and mess up some of the types... The added unit test should catch that though.
} | ||
} | ||
|
||
extension Dictionary { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the tests are fixed and new tests added, this can probably be simplified so that you only write the implementations for NSDictionary or Swift dictionary and then cast to the other type. That way you don't need to duplicate the implementations.
For sure it can be done for the caseInsensitiveKey
and value(forCaseInsensitiveKey key: String)
methods, but transformValuesToString
may have issues due to bridging messing up the types, though it would probably still be fine if the Swift implementation cases to NSDictionary vs the other way around.
Summary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)