Conversation
Codecov Report
@@ Coverage Diff @@
## master #2 +/- ##
==========================================
- Coverage 92.5% 87.62% -4.88%
==========================================
Files 4 3 -1
Lines 40 97 +57
Branches 3 0 -3
==========================================
+ Hits 37 85 +48
- Misses 0 12 +12
+ Partials 3 0 -3
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| public extension ObservableType where E: Encodable { | ||
| public func toDictionary() -> Observable<[String:Any]> { |
There was a problem hiding this comment.
- How about renaming it to
mapDictionary()? - I think we need a parameter of
JSONEncoder.map()takes an instance ofJSONDecoderas a parameter. - Could you please append a whitespace after a colon?
- [String:Any] + [String: Any]
- How do you think of supporting mapping json array?
There was a problem hiding this comment.
- like toBlocking(), toArray(). toXXX() feels extract some data. toDictionary() is more clear for me, aren't you? ( I understand your side, changing data should start with map... )
- Yes. It seems necessary
- my mistake.
- why not
There was a problem hiding this comment.
- It's not about extracting data. It's obviously mapping data. I think it would be better to rename it to
mapDictionary(). - ✅
- ✅
- Could you write that one?
There was a problem hiding this comment.
- then
toJSONString()needs rename tomapJSONString(), too?
| return self.map { encodable -> [String: Any] in | ||
| let data = try JSONEncoder().encode(encodable) | ||
| let dictionary = try JSONSerialization.jsonObject(with: data) as? [String: Any] | ||
| return dictionary ?? [:] |
There was a problem hiding this comment.
This default value can cause an unexpected result. I think we should treat a casting failure as an error.
There was a problem hiding this comment.
good point!
access empty dictionary makes crash.. it would be better throw error
unit test testMapCodableArrayToJSONString() added : [Encodable] -> toJSONString()
|
| return self.map { encodable -> [String: Any] in | ||
| let data = try (encoder ?? JSONEncoder()).encode(encodable) | ||
| let dict = try JSONSerialization.jsonObject(with: data) as? [String: Any] | ||
| guard let dictionary = dict else { throw RxError.noElements } |
There was a problem hiding this comment.
RxError.noElements should not be used here. It would be better to create a new error type for RxCodable.
There was a problem hiding this comment.
how about this?
enum RxCodableError : Error, CustomDebugStringConvertible {
case decodeFail
case encodeFail
var debugDescription: String {
switch self {
case .decodeFail:
return "Encoding failure"
case .encodeFail:
return "Decoding failure"
}
}
}There was a problem hiding this comment.
We should give more detailed information. And CustomDebugStringConvertible is not necessary.
enum RxCodableError: Error {
case castingFailure(Any, Any.Type)
}You can refer to RxJSONError.
| return self.map { encodable -> String in | ||
| let data = try (encoder ?? JSONEncoder()).encode(encodable) | ||
| let json = String(data: data, encoding: encoding) | ||
| return json ?? "{}" |
There was a problem hiding this comment.
Please don't use default value here.
RxCodableError added. thows mapDictionary, mapJSONString feature added that mapping [Dictionary] ==> [Encodable]
| } | ||
|
|
||
| public extension ObservableType where E == [[String: Any]] { | ||
| public func map<T>(_ type: Array<T>.Type, using decoder: JSONDecoder? = nil) -> Observable<[T]> where T: Decodable { |
There was a problem hiding this comment.
What is this? The 'array mapping' I meant was about func mapArray() which maps Observable<[T]> to Observable<[[String: Any]]>.
There was a problem hiding this comment.
Oh.. my misunderstood. now I see.
There was a problem hiding this comment.
Hmm.. It's not as easy as I thought.
4 participants
My project using Firebase, needs convertings between Model and Dictionary, [String:Any].
So I added "toDictionary()"
and "toJSONString()" for POST request