TDOAuth.swift refinement #44
Conversation
Source/compat/TDOAuth.swift
Outdated
| private class func getStringValue(by rawValue: Any) -> String? { | ||
| var formattedValue: String? | ||
| switch rawValue { | ||
| case let losslessString as LosslessStringConvertible: |
There was a problem hiding this comment.
String conforms LosslessStringConvertible per the docs, as do several of the rest. This one should be after String and other direct conversions or they’ll never be called. it’s not clear to me if the new lossless convertible for something like bool is better/worse than the dedicated initializer. Maybe we can remove most of the cases here.
| case let boolValue as Bool: | ||
| formattedValue = String(boolValue) | ||
| case let arrayValue as NSArray: | ||
| formattedValue = String(describing: arrayValue) |
There was a problem hiding this comment.
Nice! My only concern with this is that there is no intent captured. Without an output contract here it seems possible that future implementations could break. I think it would interesting to add an extension of “Array where Element conforms to LosslessStringConvertible”, which conforms the array to the same protocol by concatenating in the exact format needed. This has the added benefit that you wouldn’t need a special case here.
But, this is good for now.
There was a problem hiding this comment.
Thanks, @adamkaplan. Previously I kept those cases of NSNumber, String and Bool because LosslessStringConvertible doesn't take care of some Obj-C object types. Finally, my team provides me a better way of using NSObjectProtocol.
Please see if this is good to you.
|
@adamkaplan , @stury |
|
@WinnieLYT Can you also update the version number in the podspec so we have a new patch release? This will make it simpler to roll out the change for you. |
|
@stury done. |
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.
This PR is to fix:
TDOQueryItem.httpBodyfailureTDOQueryItemto make sure the type casting is correct.